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