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:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > 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.
> > > > Try this one:
> > > >
> > > > ```
> > > > struct Obj {
> > > > int a;
> > > > Obj();
> > > > ~Obj();
> > > > };
> > > >
> > > > void foo() {
> > > > #pragma omp parallel
> > > > for (int i=0; i<1000; i++) {
> > > > if(i==100) {
> > > > Obj obj;
> > > > #pragma omp cancel parallel
> > > > #pragma omp barrier
> > > > }
> > > > }
> > > > }
> > > > ```
> > > Same result, cancel semantic is unaffected. Are you trying these?
> > There must be different code for _kmpc_cancel_barrier call and further
> > processing. Will try to check with your patch tomorrow.
> Ok, I see, you're using the block that jumps through the cleanups. Ok, this
> seems good.
>
Yes. I also used the same logic in the generic solution (D70258) that will work
with both the old code gen and the new one, e.g., D70109.
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
