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

[Alexey Bataev via Phabricator via cfe-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785