ABataev added inline comments.
================ Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:200 +OpenMPIRBuilder::InsertPointTy +OpenMPIRBuilder::emitBarrierImpl(const LocationDescription &Loc, Directive Kind, + bool ForceSimpleCall, bool CheckCancelFlag) { ---------------- Maybe split emission ща `__kmpc_barrier` and `__kmpc_cancel_barrier` functions into 2 independent functions fo the frontends? Rather than rely on the boolean flags? ================ Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228 + getOrCreateThreadID(getOrCreateIdent(SrcLocStr))}; + bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock; + Value *Result = Builder.CreateCall( ---------------- 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. 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