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