jdoerfert marked an inline comment as done.
jdoerfert added inline comments.
================
Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228
+ getOrCreateThreadID(getOrCreateIdent(SrcLocStr))};
+ bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+ Value *Result = Builder.CreateCall(
----------------
ABataev wrote:
> ABataev wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdoerfert wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > Maybe add an assert when the cancellation version is
> > > > > > > > > > > > > requested but the cancellation block is not set?
> > > > > > > > > > > > > Instead of the generating simple version of barrier.
> > > > > > > > > > > > The interface doesn't work that way as we do not know
> > > > > > > > > > > > here if the cancellation was requested except if the
> > > > > > > > > > > > block was set. That is basically the flag (and I expect
> > > > > > > > > > > > it to continue to be that way).
> > > > > > > > > > > Maybe instead of `ForceSimpleBarrier` add a flag
> > > > > > > > > > > `EmitCancelBarrier` and if it set to true, always emit
> > > > > > > > > > > cancel barrier, otherwise always emit simple barrier? And
> > > > > > > > > > > add an assertion for non-set cancellation block or even
> > > > > > > > > > > accept it as a parameter here.
> > > > > > > > > > >
> > > > > > > > > > > Also, what if we have inner exception handling in the
> > > > > > > > > > > region? Will you handle the cleanup correctly in case of
> > > > > > > > > > > the cancelation barrier?
> > > > > > > > > > > Maybe instead of ForceSimpleBarrier add a flag
> > > > > > > > > > > EmitCancelBarrier and if it set to true, always emit
> > > > > > > > > > > cancel barrier, otherwise always emit simple barrier? And
> > > > > > > > > > > add an assertion for non-set cancellation block or even
> > > > > > > > > > > accept it as a parameter here.
> > > > > > > > > >
> > > > > > > > > > What is the difference in moving some of the boolean logic
> > > > > > > > > > to the caller? Also, we have test to verify we get
> > > > > > > > > > cancellation barriers if we need them, both unit tests and
> > > > > > > > > > clang lit tests.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Also, what if we have inner exception handling in the
> > > > > > > > > > > region? Will you handle the cleanup correctly in case of
> > > > > > > > > > > the cancelation barrier?
> > > > > > > > > >
> > > > > > > > > > I think so. Right now through the code in clang that does
> > > > > > > > > > the set up of the cancellation block, later through
> > > > > > > > > > callbacks but we only need that for regions where we
> > > > > > > > > > actually go out of some scope, e.g., parallel.
> > > > > > > > > 1. I'm just thinking about future users of thus interface. It
> > > > > > > > > woild be good if we could provide safe interface for all the
> > > > > > > > > users, not only clang.
> > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But you may
> > > > > > > > > have inner try...catch or just simple compound statement with
> > > > > > > > > local vars that require constructors/destructors. And the
> > > > > > > > > cancellation barrier may exit out of these regions. And you
> > > > > > > > > need to call all required destructors. You'd better to think
> > > > > > > > > about it now, not later.
> > > > > > > > > 2. [...] You'd better to think about it now, not later.
> > > > > > > >
> > > > > > > > First, I do think about it now and I hope this was not an
> > > > > > > > insinuation to suggest otherwise.
> > > > > > > >
> > > > > > > > > 1. I'm just thinking about future users of thus interface. It
> > > > > > > > > woild be good if we could provide safe interface for all the
> > > > > > > > > users, not only clang.
> > > > > > > > > 2. Exit out of the OpenMP region is not allowed. But you may
> > > > > > > > > have inner try...catch or just simple compound statement with
> > > > > > > > > local vars that require constructors/destructors. And the
> > > > > > > > > cancellation barrier may exit out of these regions. And you
> > > > > > > > > need to call all required destructors.
> > > > > > > >
> > > > > > > > Generally speaking, we shall not add features that we cannot
> > > > > > > > use or test with the assumption we will use them in the future.
> > > > > > > > This is suggested by the LLVM best practices. If you have
> > > > > > > > specific changes in mind that are testable and better than what
> > > > > > > > I suggested so far, please bring them forward. You can also
> > > > > > > > bring forward suggestions on how it might look in the future
> > > > > > > > but without a real use case now it is not practical to block a
> > > > > > > > review based on that, given that we can change the interface
> > > > > > > > once the time has come.
> > > > > > > >
> > > > > > > > I said before, we will need callbacks for destructors, actual
> > > > > > > > handling of cancellation blocks, and there are various other
> > > > > > > > features missing right now. Nevertheless, we cannot build them
> > > > > > > > into the current interface, or even try to prepare for all of
> > > > > > > > them, while keeping the patches small and concise.
> > > > > > > It won't work for clang, I'm afraid. You need a list of
> > > > > > > desructors here. But clang uses recursive codegen and it is very
> > > > > > > hard to walk over the call tree and gather all required
> > > > > > > destructors into a list. At least, it will require significant
> > > > > > > rework in clang frontend.
> > > > > > > Instead of generating the branch to cancellation block in the
> > > > > > > builder, I would suggest to call a single callback function
> > > > > > > provided by the frontend, which will generate correct branch over
> > > > > > > a chain of the destructor blocks. In this case, you won't need
> > > > > > > this cancellation block at all. This is what I meant when said
> > > > > > > that you need to think about this problem right now. That current
> > > > > > > solution is not very suitable for the use in the frontend.
> > > > > > > It won't work for clang,
> > > > > >
> > > > > > It won't work in the future or it does not work now? If the latter,
> > > > > > do you have a mwe to show the problem?
> > > > > 1. Both.
> > > > > 2. What is mwe? Sure, will simple test tomorrow.
> > > > both what?
> > > > A simple test is what I wanted, thx.
> > > Both - it won't work now and in tbe future it is going to be very hard to
> > > adapt clang to this interface.
> > I mean, handling of the cleanups.
> As an example, you can take a look at the code in
> `clang/test/OpenMP/cancel_codegen_cleanup.cpp` test. It is very simple. The
> simplest version of the same code will something like this:
> ```
> struct Obj {
> int a;
> Obj();
> ~Obj();
> };
>
> void foo() {
> #pragma omp for
> for (int i=0; i<1000; i++) {
> if(i==100) {
> Obj obj;
> #pragma omp cancel for
> }
> }
> }
>
> ```
> The object `obj` won't be deleted correctly with your scheme.
How did you run/compare this to come to the conclusion it does not work?
I run it with the OpenMPIRBuilder for barriers enabled (D69922 +
-fopenmp-enable-irbuilder) and without, here is the full diff:
```
-declare dso_local void @__kmpc_barrier(%struct.ident_t*, i32)
+declare void @__kmpc_barrier(%struct.ident_t*, i32)
```
I don't see what you mean by it doesn't work, looks fine to me.
---
The above notwithstanding, if you have examples that expose problems with this
patch, please let me know.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69785/new/
https://reviews.llvm.org/D69785
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
