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