[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

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:
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69785/new/

https://reviews.llvm.org/D69785