[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(
----------------
ABataev wrote:
> 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.
As an example, you can take a look at the code in `clang/test/OpenMP/cancel_codegen_cleanup.cpp` test. It is very simple. The simplest version of the same code will something like this:
```
struct Obj {
  int a;
  Obj();
  ~Obj();
};

void foo() {
      #pragma omp for
      for (int i=0; i<1000; i++) {
            if(i==100) {
                Obj obj;
                #pragma omp cancel for
            }
        }
}

```
The object `obj` won't be deleted correctly with your scheme.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785