ABataev added inline comments.
================ Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228 + getOrCreateThreadID(getOrCreateIdent(SrcLocStr))}; + bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock; + Value *Result = Builder.CreateCall( ---------------- 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 } } } ``` 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