[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf9c3c5da19ab: [OpenMP][IR-Builder] Introduce the 
finalization stack (authored by jdoerfert).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -104,7 +104,15 @@
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
   new UnreachableInst(Ctx, CBB);
-  OMPBuilder.setCancellationBlock(CBB);
+  auto FiniCB = [CBB](llvm::OpenMPIRBuilder::InsertPointTy IP) {
+assert(IP.getBlock()->end() == IP.getPoint() &&
+   "Clang CG should cause non-terminated block!");
+BranchInst::Create(CBB, IP.getBlock());
+  };
+  // Emulate an outer parallel.
+  llvm::OpenMPIRBuilder::FinalizationInfo FI(
+  {FiniCB, OMPD_parallel, /* HasCancel */ true});
+  OMPBuilder.pushFinalizationCB(std::move(FI));
 
   IRBuilder<> Builder(BB);
 
@@ -113,7 +121,7 @@
   Builder.restoreIP(NewIP);
   EXPECT_FALSE(M->global_empty());
   EXPECT_EQ(M->size(), 3U);
-  EXPECT_EQ(F->size(), 3U);
+  EXPECT_EQ(F->size(), 4U);
   EXPECT_EQ(BB->size(), 4U);
 
   CallInst *GTID = dyn_cast(>front());
@@ -133,10 +141,15 @@
   Instruction *BarrierBBTI = Barrier->getParent()->getTerminator();
   EXPECT_EQ(BarrierBBTI->getNumSuccessors(), 2U);
   EXPECT_EQ(BarrierBBTI->getSuccessor(0), NewIP.getBlock());
-  EXPECT_EQ(BarrierBBTI->getSuccessor(1), CBB);
+  EXPECT_EQ(BarrierBBTI->getSuccessor(1)->getTerminator()->getNumSuccessors(),
+1U);
+  EXPECT_EQ(BarrierBBTI->getSuccessor(1)->getTerminator()->getSuccessor(0),
+CBB);
 
   EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
 
+  OMPBuilder.popFinalizationCB();
+
   Builder.CreateUnreachable();
   EXPECT_FALSE(verifyModule(*M));
 }
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -205,7 +205,8 @@
   // If we are in a cancellable parallel region, barriers are cancellation
   // points.
   // TODO: Check why we would force simple calls or to ignore the cancel flag.
-  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  bool UseCancelBarrier =
+  !ForceSimpleCall && isLastFinalizationInfoCancellable(OMPD_parallel);
 
   Value *Result = Builder.CreateCall(
   getOrCreateRuntimeFunction(UseCancelBarrier ? OMPRTL___kmpc_cancel_barrier
@@ -217,19 +218,22 @@
 BasicBlock *BB = Builder.GetInsertBlock();
 BasicBlock *NonCancellationBlock = BasicBlock::Create(
 BB->getContext(), BB->getName() + ".cont", BB->getParent());
+BasicBlock *CancellationBlock = BasicBlock::Create(
+BB->getContext(), BB->getName() + ".cncl", BB->getParent());
 
 // Jump to them based on the return value.
 Value *Cmp = Builder.CreateIsNull(Result);
 Builder.CreateCondBr(Cmp, NonCancellationBlock, CancellationBlock,
  /* TODO weight */ nullptr, nullptr);
 
-Builder.SetInsertPoint(NonCancellationBlock);
-assert(CancellationBlock->getParent() == BB->getParent() &&
-   "Unexpected cancellation block parent!");
+// From the cancellation block we finalize all variables and go to the
+// post finalization block that is known to the FiniCB callback.
+Builder.SetInsertPoint(CancellationBlock);
+auto  = FinalizationStack.back();
+FI.FiniCB(Builder.saveIP());
 
-// TODO: This is a workaround for now, we always reset the cancellation
-// block until we manage it ourselves here.
-CancellationBlock = nullptr;
+// The continuation block is where code generation continues.
+Builder.SetInsertPoint(NonCancellationBlock);
   }
 
   return Builder.saveIP();
Index: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
===
--- llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
+++ llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
@@ -37,12 +37,44 @@
   /// Add attributes known for \p FnID to \p Fn.
   void addAttributes(omp::RuntimeFunction FnID, Function );
 
-  /// Set the cancellation block to \p CBB.
-  void setCancellationBlock(BasicBlock *CBB) { CancellationBlock = CBB; }
-
   /// Type used throughout for insertion points.
   using InsertPointTy = IRBuilder<>::InsertPoint;
 
+  /// Callback type for variable finalization (think destructors).
+  ///
+  /// \param CodeGenIP is the insertion point at which the finalization code
+  ///  should be placed.
+  ///
+  

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Okay, if this is just handling OpenMP-specific control flow and doesn't need to 
interact with what a reasonable frontend would do with cleanups,  I'm appeased.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D70258#1788861 , @rjmccall wrote:

> That's what I figured.


Just to say this again:
Current OpenMP code generation keeps a second stack for exactly the same 
purpose as the one introduced here.

> The thing is that that necessarily interacts correctly with everything in 
> Clang's IRGen in ways that making a second stack that Clang's IRGen doesn't 
> directly know about doesn't.
> 
> I think you either need to extract Clang's entire cleanup-stack concept into 
> a generic frontend library, so that both Clang and your generic OpenMP 
> lowering just manipulate that common stack, or you need to call back into the 
> frontend to manage your cleanup.

I like the idea of a generic way to do this but I'm unsure if that is needed 
given the restrictions on OpenMP regions. We basically are not allowed to jump 
out by non-OpenMP means. What we do in the existing, and this rewritten stack 
is remember what OpenMP means will jump out of the current scope and to the end 
of the region. This can be reused in Flang as the restrictions hold there as 
well (as far as I know).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D70258#1788825 , @rjmccall wrote:

> So it's never that OpenMP has things that need to be finalized before exiting 
> (e.g. if something in frontend-emitted code throws an exception), it's just 
> that OpenMP might need to generate its own control flow that breaks through 
> arbitrary scopes in the frontend?


Depending on how you look at it. OpenMP has to finalize some constructs, which 
usually means to place a call before we continue with the code that follows. It 
also introduce a new scope in which things like privatization happen, however 
these are almost completely handled through clang (see below). The one thing 
that comes to mind is `lastprivate` which is handled by OpenMP.

Btw. Exceptions have to be caught inside an OpenMP scope that has a finalizer 
as it would be UB otherwise (as with the `return`).

> I would really hope that the OpenMP implementation in Clang would've used 
> Clang's cleanup stack rather than inventing its own mechanism.

This patch uses (parts of) clangs cleanup logic and the existing CGOpenMP 
cleanup code (which is the existing OpenMP specific stack 
(`CodeGenFunction::OMPCancelStack`) this one will replace).

---

In the code that glues the old CGOpenMP to the new OMPBuilder, where we create 
the finalization callback, the existing `getOMPCancelDestination` is used as 
shown below.

  CodeGenFunction::JumpDest Dest = CGF.getOMPCancelDestination(OMPD_parallel);
  CGF.EmitBranchThroughCleanup(Dest);

In the follow up patch (D70290 ) that will 
lower `#pragma omp parallel` through the OMPIRBuilder, we don't need the 
`clang::CodeGenFunction::OMPCancelDestination`/`clang::CodeGenFunction::OMPCancelStack`
 stuff anymore (for the parallel) and the finalization callback becomes:

  CodeGenFunction::JumpDest Dest = getJumpDestInCurrentScope(DestBB);
  EmitBranchThroughCleanup(Dest);

With `DestBB` defined as the end of the OpenMP region/scope.

---

The additional stack is also needed because depending on the nesting we may or 
may not create control flow that can jump to the end of a scope. So

  #pragma omp parallel
  {
{
  ... ;
  #pragma omp barrier // or an implicit barrier
  ... ;
}
parallel_exit:
  }

will cause a control flow edge from the barrier to `parallel_exit`. This edge 
may not exist if there is another OpenMP region nested in the parallel.

---

In D70258#1788854 , @ABataev wrote:

> In D70258#1788825 , @rjmccall wrote:
>
> > I would really hope that the OpenMP implementation in Clang would've used 
> > Clang's cleanup stack rather than inventing its own mechanism.
>
>
> John, current implementation completely relies on Clang's cleanup stack.


Let me rephrase the above statement less misleading:
The current implementation relies on Clang's cleanup stack *and* the OpenMP 
specific `clang::CodeGenFunction::OMPCancelStack`, which is what this patch 
will eventually replace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

That's what I figured.  The thing is that that necessarily interacts correctly 
with everything in Clang's IRGen in ways that making a second stack that 
Clang's IRGen doesn't directly know about doesn't.

I think you either need to extract Clang's entire cleanup-stack concept into a 
generic frontend library, so that both Clang and your generic OpenMP lowering 
just manipulate that common stack, or you need to call back into the frontend 
to manage your cleanup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.



In D70258#1788825 , @rjmccall wrote:

> So it's never that OpenMP has things that need to be finalized before exiting 
> (e.g. if something in frontend-emitted code throws an exception), it's just 
> that OpenMP might need to generate its own control flow that breaks through 
> arbitrary scopes in the frontend?
>
> I would really hope that the OpenMP implementation in Clang would've used 
> Clang's cleanup stack rather than inventing its own mechanism.


John, current implementation completely relies on Clang's cleanup stack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

So it's never that OpenMP has things that need to be finalized before exiting 
(e.g. if something in frontend-emitted code throws an exception), it's just 
that OpenMP might need to generate its own control flow that breaks through 
arbitrary scopes in the frontend?

I would really hope that the OpenMP implementation in Clang would've used 
Clang's cleanup stack rather than inventing its own mechanism.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D70258#1788427 , @rjmccall wrote:

> In D70258#1788396 , @jdoerfert wrote:
>
> > In D70258#1788305 , @rjmccall 
> > wrote:
> >
> > > Introducing an IRBuilder-level finalization stack sounds like it's going 
> > > to be a huge mess if your goal is to plug this into other frontends.
> >
> >
> > While I get that you don't want to review this, I would really like to 
> > understand why you think this would become a mess.
>
>
> I guess it depends on what you're expecting to be able to achieve with this 
> stack.  Frontends have their own notion of what needs to be finalized and 
> what can trigger control flow.  If your finalization stack is purely for the 
> convenience of your internal IR-generation, it's fine.  If the need for a 
> finalizer can cross the emission of arbitrary frontend code, or if your code 
> needs to emit branches that might cross arbitrary frontend "scope" 
> boundaries, you're going to be in trouble.


Let me explain what it does and why, maybe that helps:

The "finalizer" is a frontend provided callback as only the frontend "knows" 
what values to finalize and "how" to finalize them if we leave the scope. It is 
created only for OpenMP scopes as we only manage leaving those in the 
OMPBuilder. If we generate code for an OpenMP directive that will cause control 
flow to leave the region the finalizer is invoked. Other language finalization, 
e.g., because of nested control flow, will happen undisturbed. It works because 
OpenMP restricts what can happen in a region, e.g. you cannot return from an 
OpenMP parallel region, so all exits have to be "natural" or OpenMP related.

FWIW: We already have (basically) the same scheme in Clang, managed by the 
CGOpenMP and doing basically the same thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D70258#1788396 , @jdoerfert wrote:

> In D70258#1788305 , @rjmccall wrote:
>
> > Introducing an IRBuilder-level finalization stack sounds like it's going to 
> > be a huge mess if your goal is to plug this into other frontends.
>
>
> While I get that you don't want to review this, I would really like to 
> understand why you think this would become a mess.


I guess it depends on what you're expecting to be able to achieve with this 
stack.  Frontends have their own notion of what needs to be finalized and what 
can trigger control flow.  If your finalization stack is purely for the 
convenience of your internal IR-generation, it's fine.  If the need for a 
finalizer can cross the emission of arbitrary frontend code, or if your code 
needs to emit branches that might cross arbitrary frontend "scope" boundaries, 
you're going to be in trouble.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a subscriber: rjmccall.
jdoerfert added a comment.

In D70258#1788305 , @rjmccall wrote:

> I opposed the creation of this library on these terms in the first place, so 
> I'm pretty sure I'm not on the hook to review it.


That's fine with me.

> Introducing an IRBuilder-level finalization stack sounds like it's going to 
> be a huge mess if your goal is to plug this into other frontends.

While I get that you don't want to review this, I would really like to 
understand why you think this would become a mess.

FWIW. The only code that is currently in Clang to make this work is going to be 
removed once the code generation is moved as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[John McCall via Phabricator via cfe-commits]

rjmccall resigned from this revision.
rjmccall added a comment.

I opposed the creation of this library on these terms in the first place, so 
I'm pretty sure I'm not on the hook to review it.  Introducing an 
IRBuilder-level finalization stack sounds like it's going to be a huge mess if 
your goal is to plug this into other frontends.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+

hfinkel wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > jdoerfert wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > `llvm::function_ref`?
> > > > > > > > The lambda that is passed in here might go out of scope so we 
> > > > > > > > need to own it. This is intentional.
> > > > > > > Maybe better to save the intermediate data in CGOpenMPRuntime 
> > > > > > > class rather than rely on owning lambda ref here? Clang does not 
> > > > > > > use escaping decls and instead stores intermediate data 
> > > > > > > explicitly. It really complicates analysis and potential source 
> > > > > > > of resource leakage.
> > > > > > I don't follow. Clang does use `std::function` to store callbacks 
> > > > > > that have to life for a while. Why is this different? What would be 
> > > > > > the benefit of having a function_ref here and a `std::function` in 
> > > > > > CGOpenMPRuntime? Note that the FinaliztionInfo object is created in 
> > > > > > the front-end and the std::function is assigned there already.
> > > > > Clang uses it only in some rare cases, when it is really required, 
> > > > > like typo correction or something like this. In other cases it is not 
> > > > > recommended to use it.
> > > > > 
> > > > > I'm not saying that you need to store `std::function` in 
> > > > > CGOpenMPRuntime class, I'm saying about necessary data.
> > > > What necessary data? Can you please explain how you want it to look 
> > > > like and why? This version seems to work fine.
> > > I'm not arguing that it does not work, I'm asking do you really need such 
> > > a complex solution? Just like I said before, it is very hard to maintain 
> > > and to understand the lifetime of lambda, so it is a potential source of 
> > > resource leakage. All I'm asking is `is there a way to implement it 
> > > differently`, nothing else.
> > There are alternatives, all of which are more complex and come with the 
> > same "problems".
> @jdoerfert , can you please elaborate? What other designs might be possible?
> 
> It looks to me like this lambda is necessary to maintain separation between 
> Clang's CodeGen and the OpenMPIRBuilder (the non-unit-test use in this patch 
> only captures CGF). I'm not particularly concerned about lifetime management 
> of the lambdas if they only need to capture CGF, and maybe the design of this 
> implies that the lambdas will only capture objects that live as long as the 
> code generation phase, but it is certainly true that whenever we have 
> long-lived lambdas thought about lifetime of captured data is required.
I'm confused. It seems people dislike the callback for potential risks even 
though, apparently, no one sees an actual problem nor an alternative without 
callbacks.

The alternative I mentioned before and the initial implementation were:
 - Initially I only had a function_ref here, which should work for now, but I 
doubt that this is any better.
 - Having a std::function in the frontend and a function_ref here. Though, that 
only doubles the problems.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > `llvm::function_ref`?
> > > > > > > The lambda that is passed in here might go out of scope so we 
> > > > > > > need to own it. This is intentional.
> > > > > > Maybe better to save the intermediate data in CGOpenMPRuntime class 
> > > > > > rather than rely on owning lambda ref here? Clang does not use 
> > > > > > escaping decls and instead stores intermediate data explicitly. It 
> > > > > > really complicates analysis and potential source of resource 
> > > > > > leakage.
> > > > > I don't follow. Clang does use `std::function` to store callbacks 
> > > > > that have to life for a while. Why is this different? What would be 
> > > > > the benefit of having a function_ref here and a `std::function` in 
> > > > > CGOpenMPRuntime? Note that the FinaliztionInfo object is created in 
> > > > > the front-end and the std::function is assigned there already.
> > > > Clang uses it only in some rare cases, when it is really required, like 
> > > > typo correction or something like this. In other cases it is not 
> > > > recommended to use it.
> > > > 
> > > > I'm not saying that you need to store `std::function` in 
> > > > CGOpenMPRuntime class, I'm saying about necessary data.
> > > What necessary data? Can you please explain how you want it to look like 
> > > and why? This version seems to work fine.
> > I'm not arguing that it does not work, I'm asking do you really need such a 
> > complex solution? Just like I said before, it is very hard to maintain and 
> > to understand the lifetime of lambda, so it is a potential source of 
> > resource leakage. All I'm asking is `is there a way to implement it 
> > differently`, nothing else.
> There are alternatives, all of which are more complex and come with the same 
> "problems".
@jdoerfert , can you please elaborate? What other designs might be possible?

It looks to me like this lambda is necessary to maintain separation between 
Clang's CodeGen and the OpenMPIRBuilder (the non-unit-test use in this patch 
only captures CGF). I'm not particularly concerned about lifetime management of 
the lambdas if they only need to capture CGF, and maybe the design of this 
implies that the lambdas will only capture objects that live as long as the 
code generation phase, but it is certainly true that whenever we have 
long-lived lambdas thought about lifetime of captured data is required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > `llvm::function_ref`?
> > > > > > The lambda that is passed in here might go out of scope so we need 
> > > > > > to own it. This is intentional.
> > > > > Maybe better to save the intermediate data in CGOpenMPRuntime class 
> > > > > rather than rely on owning lambda ref here? Clang does not use 
> > > > > escaping decls and instead stores intermediate data explicitly. It 
> > > > > really complicates analysis and potential source of resource leakage.
> > > > I don't follow. Clang does use `std::function` to store callbacks that 
> > > > have to life for a while. Why is this different? What would be the 
> > > > benefit of having a function_ref here and a `std::function` in 
> > > > CGOpenMPRuntime? Note that the FinaliztionInfo object is created in the 
> > > > front-end and the std::function is assigned there already.
> > > Clang uses it only in some rare cases, when it is really required, like 
> > > typo correction or something like this. In other cases it is not 
> > > recommended to use it.
> > > 
> > > I'm not saying that you need to store `std::function` in CGOpenMPRuntime 
> > > class, I'm saying about necessary data.
> > What necessary data? Can you please explain how you want it to look like 
> > and why? This version seems to work fine.
> I'm not arguing that it does not work, I'm asking do you really need such a 
> complex solution? Just like I said before, it is very hard to maintain and to 
> understand the lifetime of lambda, so it is a potential source of resource 
> leakage. All I'm asking is `is there a way to implement it differently`, 
> nothing else.
There are alternatives, all of which are more complex and come with the same 
"problems".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > `llvm::function_ref`?
> > > > > The lambda that is passed in here might go out of scope so we need to 
> > > > > own it. This is intentional.
> > > > Maybe better to save the intermediate data in CGOpenMPRuntime class 
> > > > rather than rely on owning lambda ref here? Clang does not use escaping 
> > > > decls and instead stores intermediate data explicitly. It really 
> > > > complicates analysis and potential source of resource leakage.
> > > I don't follow. Clang does use `std::function` to store callbacks that 
> > > have to life for a while. Why is this different? What would be the 
> > > benefit of having a function_ref here and a `std::function` in 
> > > CGOpenMPRuntime? Note that the FinaliztionInfo object is created in the 
> > > front-end and the std::function is assigned there already.
> > Clang uses it only in some rare cases, when it is really required, like 
> > typo correction or something like this. In other cases it is not 
> > recommended to use it.
> > 
> > I'm not saying that you need to store `std::function` in CGOpenMPRuntime 
> > class, I'm saying about necessary data.
> What necessary data? Can you please explain how you want it to look like and 
> why? This version seems to work fine.
I'm not arguing that it does not work, I'm asking do you really need such a 
complex solution? Just like I said before, it is very hard to maintain and to 
understand the lifetime of lambda, so it is a potential source of resource 
leakage. All I'm asking is `is there a way to implement it differently`, 
nothing else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > `llvm::function_ref`?
> > > > The lambda that is passed in here might go out of scope so we need to 
> > > > own it. This is intentional.
> > > Maybe better to save the intermediate data in CGOpenMPRuntime class 
> > > rather than rely on owning lambda ref here? Clang does not use escaping 
> > > decls and instead stores intermediate data explicitly. It really 
> > > complicates analysis and potential source of resource leakage.
> > I don't follow. Clang does use `std::function` to store callbacks that have 
> > to life for a while. Why is this different? What would be the benefit of 
> > having a function_ref here and a `std::function` in CGOpenMPRuntime? Note 
> > that the FinaliztionInfo object is created in the front-end and the 
> > std::function is assigned there already.
> Clang uses it only in some rare cases, when it is really required, like typo 
> correction or something like this. In other cases it is not recommended to 
> use it.
> 
> I'm not saying that you need to store `std::function` in CGOpenMPRuntime 
> class, I'm saying about necessary data.
What necessary data? Can you please explain how you want it to look like and 
why? This version seems to work fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > `llvm::function_ref`?
> > > The lambda that is passed in here might go out of scope so we need to own 
> > > it. This is intentional.
> > Maybe better to save the intermediate data in CGOpenMPRuntime class rather 
> > than rely on owning lambda ref here? Clang does not use escaping decls and 
> > instead stores intermediate data explicitly. It really complicates analysis 
> > and potential source of resource leakage.
> I don't follow. Clang does use `std::function` to store callbacks that have 
> to life for a while. Why is this different? What would be the benefit of 
> having a function_ref here and a `std::function` in CGOpenMPRuntime? Note 
> that the FinaliztionInfo object is created in the front-end and the 
> std::function is assigned there already.
Clang uses it only in some rare cases, when it is really required, like typo 
correction or something like this. In other cases it is not recommended to use 
it.

I'm not saying that you need to store `std::function` in CGOpenMPRuntime class, 
I'm saying about necessary data.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 60744 tests passed, 0 failed 
and 726 were skipped.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 test-results.xml 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > `llvm::function_ref`?
> > The lambda that is passed in here might go out of scope so we need to own 
> > it. This is intentional.
> Maybe better to save the intermediate data in CGOpenMPRuntime class rather 
> than rely on owning lambda ref here? Clang does not use escaping decls and 
> instead stores intermediate data explicitly. It really complicates analysis 
> and potential source of resource leakage.
I don't follow. Clang does use `std::function` to store callbacks that have to 
life for a while. Why is this different? What would be the benefit of having a 
function_ref here and a `std::function` in CGOpenMPRuntime? Note that the 
FinaliztionInfo object is created in the front-end and the std::function is 
assigned there already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 233472.
jdoerfert marked an inline comment as done.
jdoerfert added a comment.

Update the unit test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

Index: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
===
--- llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -104,7 +104,15 @@
 
   BasicBlock *CBB = BasicBlock::Create(Ctx, "", F);
   new UnreachableInst(Ctx, CBB);
-  OMPBuilder.setCancellationBlock(CBB);
+  auto FiniCB = [CBB](llvm::OpenMPIRBuilder::InsertPointTy IP) {
+assert(IP.getBlock()->end() == IP.getPoint() &&
+   "Clang CG should cause non-terminated block!");
+BranchInst::Create(CBB, IP.getBlock());
+  };
+  // Emulate an outer parallel.
+  llvm::OpenMPIRBuilder::FinalizationInfo FI(
+  {FiniCB, OMPD_parallel, /* HasCancel */ true});
+  OMPBuilder.pushFinalizationCB(std::move(FI));
 
   IRBuilder<> Builder(BB);
 
@@ -113,7 +121,7 @@
   Builder.restoreIP(NewIP);
   EXPECT_FALSE(M->global_empty());
   EXPECT_EQ(M->size(), 3U);
-  EXPECT_EQ(F->size(), 3U);
+  EXPECT_EQ(F->size(), 4U);
   EXPECT_EQ(BB->size(), 4U);
 
   CallInst *GTID = dyn_cast(>front());
@@ -133,10 +141,15 @@
   Instruction *BarrierBBTI = Barrier->getParent()->getTerminator();
   EXPECT_EQ(BarrierBBTI->getNumSuccessors(), 2U);
   EXPECT_EQ(BarrierBBTI->getSuccessor(0), NewIP.getBlock());
-  EXPECT_EQ(BarrierBBTI->getSuccessor(1), CBB);
+  EXPECT_EQ(BarrierBBTI->getSuccessor(1)->getTerminator()->getNumSuccessors(),
+1U);
+  EXPECT_EQ(BarrierBBTI->getSuccessor(1)->getTerminator()->getSuccessor(0),
+CBB);
 
   EXPECT_EQ(cast(Barrier)->getArgOperand(1), GTID);
 
+  OMPBuilder.popFinalizationCB();
+
   Builder.CreateUnreachable();
   EXPECT_FALSE(verifyModule(*M));
 }
Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -205,7 +205,8 @@
   // If we are in a cancellable parallel region, barriers are cancellation
   // points.
   // TODO: Check why we would force simple calls or to ignore the cancel flag.
-  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  bool UseCancelBarrier =
+  !ForceSimpleCall && isLastFinalizationInfoCancellable(OMPD_parallel);
 
   Value *Result = Builder.CreateCall(
   getOrCreateRuntimeFunction(UseCancelBarrier ? OMPRTL___kmpc_cancel_barrier
@@ -217,19 +218,22 @@
 BasicBlock *BB = Builder.GetInsertBlock();
 BasicBlock *NonCancellationBlock = BasicBlock::Create(
 BB->getContext(), BB->getName() + ".cont", BB->getParent());
+BasicBlock *CancellationBlock = BasicBlock::Create(
+BB->getContext(), BB->getName() + ".cncl", BB->getParent());
 
 // Jump to them based on the return value.
 Value *Cmp = Builder.CreateIsNull(Result);
 Builder.CreateCondBr(Cmp, NonCancellationBlock, CancellationBlock,
  /* TODO weight */ nullptr, nullptr);
 
-Builder.SetInsertPoint(NonCancellationBlock);
-assert(CancellationBlock->getParent() == BB->getParent() &&
-   "Unexpected cancellation block parent!");
+// From the cancellation block we finalize all variables and go to the
+// post finalization block that is known to the FiniCB callback.
+Builder.SetInsertPoint(CancellationBlock);
+auto  = FinalizationStack.back();
+FI.FiniCB(Builder.saveIP());
 
-// TODO: This is a workaround for now, we always reset the cancellation
-// block until we manage it ourselves here.
-CancellationBlock = nullptr;
+// The continuation block is where code generation continues.
+Builder.SetInsertPoint(NonCancellationBlock);
   }
 
   return Builder.saveIP();
Index: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
===
--- llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
+++ llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
@@ -37,12 +37,44 @@
   /// Add attributes known for \p FnID to \p Fn.
   void addAttributes(omp::RuntimeFunction FnID, Function );
 
-  /// Set the cancellation block to \p CBB.
-  void setCancellationBlock(BasicBlock *CBB) { CancellationBlock = CBB; }
-
   /// Type used throughout for insertion points.
   using InsertPointTy = IRBuilder<>::InsertPoint;
 
+  /// Callback type for variable finalization (think destructors).
+  ///
+  /// \param CodeGenIP is the insertion point at which the finalization code
+  ///  should be placed.
+  ///
+  /// A finalize callback knows about all 

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+

jdoerfert wrote:
> ABataev wrote:
> > `llvm::function_ref`?
> The lambda that is passed in here might go out of scope so we need to own it. 
> This is intentional.
Maybe better to save the intermediate data in CGOpenMPRuntime class rather than 
rely on owning lambda ref here? Clang does not use escaping decls and instead 
stores intermediate data explicitly. It really complicates analysis and 
potential source of resource leakage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

In D70258#1780611 , @ABataev wrote:

> Tests?


This changes the implementation and does not add features. Thus, this is 
covered by the existing tests for the functionality, e.g. the unit tests to 
build barriers and the front-end barrier test.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+

ABataev wrote:
> `llvm::function_ref`?
The lambda that is passed in here might go out of scope so we need to own it. 
This is intentional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
console-log.txt 
,
 CMakeCache.txt 
,
 diff.json 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Tests?




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+

`llvm::function_ref`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 233462.
jdoerfert added a comment.

rebase + ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -205,7 +205,8 @@
   // If we are in a cancellable parallel region, barriers are cancellation
   // points.
   // TODO: Check why we would force simple calls or to ignore the cancel flag.
-  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  bool UseCancelBarrier =
+  !ForceSimpleCall && isLastFinalizationInfoCancellable(OMPD_parallel);
 
   Value *Result = Builder.CreateCall(
   getOrCreateRuntimeFunction(UseCancelBarrier ? OMPRTL___kmpc_cancel_barrier
@@ -217,19 +218,22 @@
 BasicBlock *BB = Builder.GetInsertBlock();
 BasicBlock *NonCancellationBlock = BasicBlock::Create(
 BB->getContext(), BB->getName() + ".cont", BB->getParent());
+BasicBlock *CancellationBlock = BasicBlock::Create(
+BB->getContext(), BB->getName() + ".cncl", BB->getParent());
 
 // Jump to them based on the return value.
 Value *Cmp = Builder.CreateIsNull(Result);
 Builder.CreateCondBr(Cmp, NonCancellationBlock, CancellationBlock,
  /* TODO weight */ nullptr, nullptr);
 
-Builder.SetInsertPoint(NonCancellationBlock);
-assert(CancellationBlock->getParent() == BB->getParent() &&
-   "Unexpected cancellation block parent!");
+// From the cancellation block we finalize all variables and go to the
+// post finalization block that is known to the FiniCB callback.
+Builder.SetInsertPoint(CancellationBlock);
+auto  = FinalizationStack.back();
+FI.FiniCB(Builder.saveIP());
 
-// TODO: This is a workaround for now, we always reset the cancellation
-// block until we manage it ourselves here.
-CancellationBlock = nullptr;
+// The continuation block is where code generation continues.
+Builder.SetInsertPoint(NonCancellationBlock);
   }
 
   return Builder.saveIP();
Index: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
===
--- llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
+++ llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
@@ -37,12 +37,44 @@
   /// Add attributes known for \p FnID to \p Fn.
   void addAttributes(omp::RuntimeFunction FnID, Function );
 
-  /// Set the cancellation block to \p CBB.
-  void setCancellationBlock(BasicBlock *CBB) { CancellationBlock = CBB; }
-
   /// Type used throughout for insertion points.
   using InsertPointTy = IRBuilder<>::InsertPoint;
 
+  /// Callback type for variable finalization (think destructors).
+  ///
+  /// \param CodeGenIP is the insertion point at which the finalization code
+  ///  should be placed.
+  ///
+  /// A finalize callback knows about all objects that need finalization, e.g.
+  /// destruction, when the scope of the currently generated construct is left
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = std::function;
+
+  struct FinalizationInfo {
+/// The finalization callback provided by the last in-flight invocation of
+/// Create for the directive of kind DK.
+FinalizeCallbackTy FiniCB;
+
+/// The directive kind of the innermost directive that has an associated
+/// region which might require finalization when it is left.
+omp::Directive DK;
+
+/// Flag to indicate if the directive is cancellable.
+bool IsCancellable;
+  };
+
+  /// Push a finalization callback on the finalization stack.
+  ///
+  /// NOTE: Temporary solution until Clang CG is gone.
+  void pushFinalizationCB(const FinalizationInfo ) {
+FinalizationStack.push_back(FI);
+  }
+
+  /// Pop the last finalization callback from the finalization stack.
+  ///
+  /// NOTE: Temporary solution until Clang CG is gone.
+  void popFinalizationCB() { FinalizationStack.pop_back(); }
+
   /// Description of a LLVM-IR insertion point (IP) and a debug/source location
   /// (filename, line, column, ...).
   struct LocationDescription {
@@ -112,6 +144,19 @@
 omp::Directive DK, bool ForceSimpleCall,
 bool CheckCancelFlag);
 
+  /// The finalization stack made up of finalize callbacks currently in-flight,
+  /// wrapped into FinalizationInfo objects that reference also the finalization
+  /// target block and the kind of cancellable directive.
+  SmallVector FinalizationStack;
+
+  /// Return true if the last entry in the finalization stack is of kind \p DK
+  /// and cancellable.
+  bool 

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 4 inline comments as done.
jdoerfert added a comment.

ping




Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71
+  void pushFinalizationCB(FinalizationInfo &) {
+FinalizationStack.emplace_back(std::move(FI));
+  }

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > Maybe `push...` for const reference and `emplace...` for variadic 
> > > > > > template (just like in standard library)?
> > > > > That would defeat the purpose of using move semantic. I can make it a 
> > > > > push but we will then have to copy and not move.
> > > > You can use it in `emplace` version while `push...` function will rely 
> > > > on copy semantics.
> > > I only call this once, why do I need multiple versions?
> > I thought that we may need it later in other cases, no?
> We only need this interface for situations where we want to test finalization 
> in the presence of cancellation points that are generated by the 
> OpenMPIRBuilder while the cancelled region is not. For example, once D70290 
> is in, we can remove the call from the barrier code (it will be part of 
> D70290). If we always port the regions first (sections, for, ...) we don't 
> need this interface at all.
I'll make this a const reference and push back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71
+  void pushFinalizationCB(FinalizationInfo &) {
+FinalizationStack.emplace_back(std::move(FI));
+  }

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > Maybe `push...` for const reference and `emplace...` for variadic 
> > > > > template (just like in standard library)?
> > > > That would defeat the purpose of using move semantic. I can make it a 
> > > > push but we will then have to copy and not move.
> > > You can use it in `emplace` version while `push...` function will rely on 
> > > copy semantics.
> > I only call this once, why do I need multiple versions?
> I thought that we may need it later in other cases, no?
We only need this interface for situations where we want to test finalization 
in the presence of cancellation points that are generated by the 
OpenMPIRBuilder while the cancelled region is not. For example, once D70290 is 
in, we can remove the call from the barrier code (it will be part of D70290). 
If we always port the regions first (sections, for, ...) we don't need this 
interface at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 229582.
jdoerfert marked an inline comment as done.
jdoerfert added a comment.

Make it a specialized push-pop RAII object (just moved code)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMPIRBuilder.h
  llvm/lib/Frontend/OpenMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMPIRBuilder.cpp
@@ -228,7 +228,8 @@
   // If we are in a cancellable parallel region, barriers are cancellation
   // points.
   // TODO: Check why we would force simple calls or to ignore the cancel flag.
-  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  bool UseCancelBarrier =
+  !ForceSimpleCall && isLastFinalizationInfoCancellable(OMPD_parallel);
 
   Value *Result = Builder.CreateCall(
   getOrCreateRuntimeFunction(UseCancelBarrier ? OMPRTL___kmpc_cancel_barrier
@@ -248,13 +249,14 @@
 Builder.CreateCondBr(Cmp, NonCancellationBlock, CancellationBlock,
  /* TODO weight */ nullptr, nullptr);
 
-Builder.SetInsertPoint(NonCancellationBlock);
-assert(CancellationBlock->getParent() == BB->getParent() &&
-   "Unexpected cancellation block parent!");
+// From the cancellation block we finalize all variables and go to the
+// post finalization block that is known to the FiniCB callback.
+Builder.SetInsertPoint(CancellationBlock);
+auto  = FinalizationStack.back();
+FI.FiniCB(Builder.saveIP());
 
-// TODO: This is a workaround for now, we always reset the cancellation
-// block until we manage it ourselves here.
-CancellationBlock = nullptr;
+// The continuation block is where code generation continues.
+Builder.SetInsertPoint(NonCancellationBlock);
   }
 
   return Builder.saveIP();
Index: llvm/include/llvm/Frontend/OpenMPIRBuilder.h
===
--- llvm/include/llvm/Frontend/OpenMPIRBuilder.h
+++ llvm/include/llvm/Frontend/OpenMPIRBuilder.h
@@ -37,12 +37,44 @@
   /// Add attributes known for \p FnID to \p Fn.
   void addAttributes(omp::RuntimeFunction FnID, Function );
 
-  /// Set the cancellation block to \p CBB.
-  void setCancellationBlock(BasicBlock *CBB) { CancellationBlock = CBB; }
-
   /// Type used throughout for insertion points.
   using InsertPointTy = IRBuilder<>::InsertPoint;
 
+  /// Callback type for variable finalization (think destructors).
+  ///
+  /// \param CodeGenIP is the insertion point at which the finalization code
+  ///  should be placed.
+  ///
+  /// A finalize callback knows about all objects that need finalization, e.g.
+  /// destruction, when the scope of the currently generated construct is left
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = function_ref;
+
+  struct FinalizationInfo {
+/// The finalization callback provided by the last in-flight invocation of
+/// Create for the directive of kind DK.
+FinalizeCallbackTy FiniCB;
+
+/// The directive kind of the innermost directive that has an associated
+/// region which might require finalization when it is left.
+omp::Directive DK;
+
+/// Flag to indicate if the directive is cancellable.
+bool IsCancellable;
+  };
+
+  /// Push a finalization callback on the finalization stack.
+  ///
+  /// NOTE: Temporary solution until Clang CG is gone.
+  void pushFinalizationCB(FinalizationInfo &) {
+FinalizationStack.emplace_back(std::move(FI));
+  }
+
+  /// Pop the last finalization callback from the finalization stack.
+  ///
+  /// NOTE: Temporary solution until Clang CG is gone.
+  void popFinalizationCB() { FinalizationStack.pop_back(); }
+
   /// Description of a LLVM-IR insertion point (IP) and a debug/source location
   /// (filename, line, column, ...).
   struct LocationDescription {
@@ -112,6 +144,19 @@
 omp::Directive DK, bool ForceSimpleCall,
 bool CheckCancelFlag);
 
+  /// The finalization stack made up of finalize callbacks currently in-flight,
+  /// wrapped into FinalizationInfo objects that reference also the finalization
+  /// target block and the kind of cancellable directive.
+  SmallVector FinalizationStack;
+
+  /// Return true if the last entry in the finalization stack is of kind \p DK
+  /// and cancellable.
+  bool isLastFinalizationInfoCancellable(omp::Directive DK) {
+return !FinalizationStack.empty() &&
+   FinalizationStack.back().IsCancellable &&
+   FinalizationStack.back().DK == DK;
+  }
+
   /// Return the current thread ID.
   ///
   /// \param Ident The ident (ident_t*) describing the query origin.
@@ -123,9 +168,6 @@
   /// 

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1481-1509
+  if (OMPBuilder) {
+
+// The following callback is the crucial part of clangs cleanup process.
+//
+// NOTE:
+// Once the OpenMPIRBuilder is used to create parallel regions (and
+// similar), the cancellation destination (Dest below) is determined via

Most of this code can be moved to the constructor of the `PopStackRAII`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71
+  void pushFinalizationCB(FinalizationInfo &) {
+FinalizationStack.emplace_back(std::move(FI));
+  }

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > Maybe `push...` for const reference and `emplace...` for variadic 
> > > > template (just like in standard library)?
> > > That would defeat the purpose of using move semantic. I can make it a 
> > > push but we will then have to copy and not move.
> > You can use it in `emplace` version while `push...` function will rely on 
> > copy semantics.
> I only call this once, why do I need multiple versions?
I thought that we may need it later in other cases, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 229573.
jdoerfert added a comment.

Replace conditional by RAII object


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMPIRBuilder.h
  llvm/lib/Frontend/OpenMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMPIRBuilder.cpp
@@ -228,7 +228,8 @@
   // If we are in a cancellable parallel region, barriers are cancellation
   // points.
   // TODO: Check why we would force simple calls or to ignore the cancel flag.
-  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  bool UseCancelBarrier =
+  !ForceSimpleCall && isLastFinalizationInfoCancellable(OMPD_parallel);
 
   Value *Result = Builder.CreateCall(
   getOrCreateRuntimeFunction(UseCancelBarrier ? OMPRTL___kmpc_cancel_barrier
@@ -248,13 +249,14 @@
 Builder.CreateCondBr(Cmp, NonCancellationBlock, CancellationBlock,
  /* TODO weight */ nullptr, nullptr);
 
-Builder.SetInsertPoint(NonCancellationBlock);
-assert(CancellationBlock->getParent() == BB->getParent() &&
-   "Unexpected cancellation block parent!");
+// From the cancellation block we finalize all variables and go to the
+// post finalization block that is known to the FiniCB callback.
+Builder.SetInsertPoint(CancellationBlock);
+auto  = FinalizationStack.back();
+FI.FiniCB(Builder.saveIP());
 
-// TODO: This is a workaround for now, we always reset the cancellation
-// block until we manage it ourselves here.
-CancellationBlock = nullptr;
+// The continuation block is where code generation continues.
+Builder.SetInsertPoint(NonCancellationBlock);
   }
 
   return Builder.saveIP();
Index: llvm/include/llvm/Frontend/OpenMPIRBuilder.h
===
--- llvm/include/llvm/Frontend/OpenMPIRBuilder.h
+++ llvm/include/llvm/Frontend/OpenMPIRBuilder.h
@@ -37,12 +37,44 @@
   /// Add attributes known for \p FnID to \p Fn.
   void addAttributes(omp::RuntimeFunction FnID, Function );
 
-  /// Set the cancellation block to \p CBB.
-  void setCancellationBlock(BasicBlock *CBB) { CancellationBlock = CBB; }
-
   /// Type used throughout for insertion points.
   using InsertPointTy = IRBuilder<>::InsertPoint;
 
+  /// Callback type for variable finalization (think destructors).
+  ///
+  /// \param CodeGenIP is the insertion point at which the finalization code
+  ///  should be placed.
+  ///
+  /// A finalize callback knows about all objects that need finalization, e.g.
+  /// destruction, when the scope of the currently generated construct is left
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = function_ref;
+
+  struct FinalizationInfo {
+/// The finalization callback provided by the last in-flight invocation of
+/// Create for the directive of kind DK.
+FinalizeCallbackTy FiniCB;
+
+/// The directive kind of the innermost directive that has an associated
+/// region which might require finalization when it is left.
+omp::Directive DK;
+
+/// Flag to indicate if the directive is cancellable.
+bool IsCancellable;
+  };
+
+  /// Push a finalization callback on the finalization stack.
+  ///
+  /// NOTE: Temporary solution until Clang CG is gone.
+  void pushFinalizationCB(FinalizationInfo &) {
+FinalizationStack.emplace_back(std::move(FI));
+  }
+
+  /// Pop the last finalization callback from the finalization stack.
+  ///
+  /// NOTE: Temporary solution until Clang CG is gone.
+  void popFinalizationCB() { FinalizationStack.pop_back(); }
+
   /// Description of a LLVM-IR insertion point (IP) and a debug/source location
   /// (filename, line, column, ...).
   struct LocationDescription {
@@ -112,6 +144,19 @@
 omp::Directive DK, bool ForceSimpleCall,
 bool CheckCancelFlag);
 
+  /// The finalization stack made up of finalize callbacks currently in-flight,
+  /// wrapped into FinalizationInfo objects that reference also the finalization
+  /// target block and the kind of cancellable directive.
+  SmallVector FinalizationStack;
+
+  /// Return true if the last entry in the finalization stack is of kind \p DK
+  /// and cancellable.
+  bool isLastFinalizationInfoCancellable(omp::Directive DK) {
+return !FinalizationStack.empty() &&
+   FinalizationStack.back().IsCancellable &&
+   FinalizationStack.back().DK == DK;
+  }
+
   /// Return the current thread ID.
   ///
   /// \param Ident The ident (ident_t*) describing the query origin.
@@ -123,9 +168,6 @@
   /// The LLVM-IR Builder used to create IR.
   IRBuilder<> Builder;
 
-  /// 

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added inline comments.



Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71
+  void pushFinalizationCB(FinalizationInfo &) {
+FinalizationStack.emplace_back(std::move(FI));
+  }

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > Maybe `push...` for const reference and `emplace...` for variadic 
> > > template (just like in standard library)?
> > That would defeat the purpose of using move semantic. I can make it a push 
> > but we will then have to copy and not move.
> You can use it in `emplace` version while `push...` function will rely on 
> copy semantics.
I only call this once, why do I need multiple versions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1499-1509
+OMPBuilder->pushFinalizationCB(std::move(FI));
+  }
+
   CGOpenMPOutlinedRegionInfo CGInfo(*CS, ThreadIDVar, CodeGen, InnermostKind,
 HasCancel, OutlinedHelperName);
   CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(CGF, );
+  llvm::Function *Fn = CGF.GenerateOpenMPCapturedStmtFunction(*CS);

jdoerfert wrote:
> ABataev wrote:
> > Again, need RAII
> There is no appropriate scope to put it in.
You can check for `OMPBuilder` in the constructor/destructor.



Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71
+  void pushFinalizationCB(FinalizationInfo &) {
+FinalizationStack.emplace_back(std::move(FI));
+  }

jdoerfert wrote:
> ABataev wrote:
> > Maybe `push...` for const reference and `emplace...` for variadic template 
> > (just like in standard library)?
> That would defeat the purpose of using move semantic. I can make it a push 
> but we will then have to copy and not move.
You can use it in `emplace` version while `push...` function will rely on copy 
semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 229448.
jdoerfert marked 5 inline comments as done.
jdoerfert added a comment.

Addressed comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMPIRBuilder.h
  llvm/lib/Frontend/OpenMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMPIRBuilder.cpp
@@ -228,7 +228,8 @@
   // If we are in a cancellable parallel region, barriers are cancellation
   // points.
   // TODO: Check why we would force simple calls or to ignore the cancel flag.
-  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  bool UseCancelBarrier =
+  !ForceSimpleCall && isLastFinalizationInfoCancellable(OMPD_parallel);
 
   Value *Result = Builder.CreateCall(
   getOrCreateRuntimeFunction(UseCancelBarrier ? OMPRTL___kmpc_cancel_barrier
@@ -248,13 +249,14 @@
 Builder.CreateCondBr(Cmp, NonCancellationBlock, CancellationBlock,
  /* TODO weight */ nullptr, nullptr);
 
-Builder.SetInsertPoint(NonCancellationBlock);
-assert(CancellationBlock->getParent() == BB->getParent() &&
-   "Unexpected cancellation block parent!");
+// From the cancellation block we finalize all variables and go to the
+// post finalization block that is known to the FiniCB callback.
+Builder.SetInsertPoint(CancellationBlock);
+auto  = FinalizationStack.back();
+FI.FiniCB(Builder.saveIP());
 
-// TODO: This is a workaround for now, we always reset the cancellation
-// block until we manage it ourselves here.
-CancellationBlock = nullptr;
+// The continuation block is where code generation continues.
+Builder.SetInsertPoint(NonCancellationBlock);
   }
 
   return Builder.saveIP();
Index: llvm/include/llvm/Frontend/OpenMPIRBuilder.h
===
--- llvm/include/llvm/Frontend/OpenMPIRBuilder.h
+++ llvm/include/llvm/Frontend/OpenMPIRBuilder.h
@@ -37,12 +37,44 @@
   /// Add attributes known for \p FnID to \p Fn.
   void addAttributes(omp::RuntimeFunction FnID, Function );
 
-  /// Set the cancellation block to \p CBB.
-  void setCancellationBlock(BasicBlock *CBB) { CancellationBlock = CBB; }
-
   /// Type used throughout for insertion points.
   using InsertPointTy = IRBuilder<>::InsertPoint;
 
+  /// Callback type for variable finalization (think destructors).
+  ///
+  /// \param CodeGenIP is the insertion point at which the finalization code
+  ///  should be placed.
+  ///
+  /// A finalize callback knows about all objects that need finalization, e.g.
+  /// destruction, when the scope of the currently generated construct is left
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = function_ref;
+
+  struct FinalizationInfo {
+/// The finalization callback provided by the last in-flight invocation of
+/// Create for the directive of kind DK.
+FinalizeCallbackTy FiniCB;
+
+/// The directive kind of the innermost directive that has an associated
+/// region which might require finalization when it is left.
+omp::Directive DK;
+
+/// Flag to indicate if the directive is cancellable.
+bool IsCancellable;
+  };
+
+  /// Push a finalization callback on the finalization stack.
+  ///
+  /// NOTE: Temporary solution until Clang CG is gone.
+  void pushFinalizationCB(FinalizationInfo &) {
+FinalizationStack.emplace_back(std::move(FI));
+  }
+
+  /// Pop the last finalization callback from the finalization stack.
+  ///
+  /// NOTE: Temporary solution until Clang CG is gone.
+  void popFinalizationCB() { FinalizationStack.pop_back(); }
+
   /// Description of a LLVM-IR insertion point (IP) and a debug/source location
   /// (filename, line, column, ...).
   struct LocationDescription {
@@ -112,6 +144,19 @@
 omp::Directive DK, bool ForceSimpleCall,
 bool CheckCancelFlag);
 
+  /// The finalization stack made up of finalize callbacks currently in-flight,
+  /// wrapped into FinalizationInfo objects that reference also the finalization
+  /// target block and the kind of cancellable directive.
+  SmallVector FinalizationStack;
+
+  /// Return true if the last entry in the finalization stack is of kind \p DK
+  /// and cancellable.
+  bool isLastFinalizationInfoCancellable(omp::Directive DK) {
+return !FinalizationStack.empty() &&
+   FinalizationStack.back().IsCancellable &&
+   FinalizationStack.back().DK == DK;
+  }
+
   /// Return the current thread ID.
   ///
   /// \param Ident The ident (ident_t*) describing the query origin.
@@ -123,9 +168,6 @@
   /// The LLVM-IR Builder used to create IR.
   

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1499-1509
+OMPBuilder->pushFinalizationCB(std::move(FI));
+  }
+
   CGOpenMPOutlinedRegionInfo CGInfo(*CS, ThreadIDVar, CodeGen, InnermostKind,
 HasCancel, OutlinedHelperName);
   CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(CGF, );
+  llvm::Function *Fn = CGF.GenerateOpenMPCapturedStmtFunction(*CS);

ABataev wrote:
> Again, need RAII
There is no appropriate scope to put it in.



Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71
+  void pushFinalizationCB(FinalizationInfo &) {
+FinalizationStack.emplace_back(std::move(FI));
+  }

ABataev wrote:
> Maybe `push...` for const reference and `emplace...` for variadic template 
> (just like in standard library)?
That would defeat the purpose of using move semantic. I can make it a push but 
we will then have to copy and not move.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1483
+// OpenMPIRBuilder is asked to construct a parallel (or similar) construct.
+auto FiniCB = [&](llvm::OpenMPIRBuilder::InsertPointTy IP) {
+  assert(IP.getBlock()->end() == IP.getPoint() &&

Better to not capture all the object implicitly by reference, provide explicit 
list



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1486
+ "Clang CG should cause non-terminated block!");
+  auto OldIP = CGF.Builder.saveIP();
+  CGF.Builder.restoreIP(IP);

Use `InsertPointGuard` where possible rather than use `saveIP/restoreIP` 
explicitly



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1490
+  CGF.getOMPCancelDestination(OMPD_parallel);
+  assert(Dest.getBlock());
+  CGF.EmitBranchThroughCleanup(Dest);

Add a message for the assert.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1499-1509
+OMPBuilder->pushFinalizationCB(std::move(FI));
+  }
+
   CGOpenMPOutlinedRegionInfo CGInfo(*CS, ThreadIDVar, CodeGen, InnermostKind,
 HasCancel, OutlinedHelperName);
   CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(CGF, );
+  llvm::Function *Fn = CGF.GenerateOpenMPCapturedStmtFunction(*CS);

Again, need RAII



Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71
+  void pushFinalizationCB(FinalizationInfo &) {
+FinalizationStack.emplace_back(std::move(FI));
+  }

Maybe `push...` for const reference and `emplace...` for variadic template 
(just like in standard library)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, 
gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim.
Herald added subscribers: cfe-commits, guansong, bollu, hiraditya.
Herald added projects: clang, LLVM.

As a permanent and generic solution to the problem of variable
finalization (destructors, lastprivate, ...), this patch introduces the
finalization stack. The objects on the stack describe (1) the
(structured) regions the OpenMP-IR-Builder is currently constructing,
(2) if these are cancellable, and (3) the callback that will perform the
finalization (=cleanup) when necessary.

As the finalization can be necessary multiple times, at different source
locations, the callback takes the position at which code is currently
generated. This position will also encode the destination of the "region
exit" block *iff* the finalization call was issues for a region
generated by the OpenMPIRBuilder. For regions generated through the old
Clang OpenMP code geneneration, the "region exit" is determined by Clang
inside the finalization call instead (see getOMPCancelDestination).

As a first user, the parallel + cancel barrier interaction is changed.
In contrast to the temporary solution before, the barrier generation in
Clang does not need to be aware of the "CancelDestination" block.
Instead, the finalization callback is and, as described above, later
even that one does not need to be.

D70109  will be updated to use this scheme.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70258

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  llvm/include/llvm/Frontend/OpenMPIRBuilder.h
  llvm/lib/Frontend/OpenMPIRBuilder.cpp

Index: llvm/lib/Frontend/OpenMPIRBuilder.cpp
===
--- llvm/lib/Frontend/OpenMPIRBuilder.cpp
+++ llvm/lib/Frontend/OpenMPIRBuilder.cpp
@@ -228,7 +228,8 @@
   // If we are in a cancellable parallel region, barriers are cancellation
   // points.
   // TODO: Check why we would force simple calls or to ignore the cancel flag.
-  bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock;
+  bool UseCancelBarrier =
+  !ForceSimpleCall && isLastFinalizationInfoCancellable(OMPD_parallel);
 
   Value *Result = Builder.CreateCall(
   getOrCreateRuntimeFunction(UseCancelBarrier ? OMPRTL___kmpc_cancel_barrier
@@ -248,13 +249,14 @@
 Builder.CreateCondBr(Cmp, NonCancellationBlock, CancellationBlock,
  /* TODO weight */ nullptr, nullptr);
 
-Builder.SetInsertPoint(NonCancellationBlock);
-assert(CancellationBlock->getParent() == BB->getParent() &&
-   "Unexpected cancellation block parent!");
+// From the cancellation block we finalize all variables and go to the
+// post finalization block that is known to the FiniCB callback.
+Builder.SetInsertPoint(CancellationBlock);
+auto  = FinalizationStack.back();
+FI.FiniCB(Builder.saveIP());
 
-// TODO: This is a workaround for now, we always reset the cancellation
-// block until we manage it ourselves here.
-CancellationBlock = nullptr;
+// The continuation block is where code generation continues.
+Builder.SetInsertPoint(NonCancellationBlock);
   }
 
   return Builder.saveIP();
Index: llvm/include/llvm/Frontend/OpenMPIRBuilder.h
===
--- llvm/include/llvm/Frontend/OpenMPIRBuilder.h
+++ llvm/include/llvm/Frontend/OpenMPIRBuilder.h
@@ -37,12 +37,44 @@
   /// Add attributes known for \p FnID to \p Fn.
   void addAttributes(omp::RuntimeFunction FnID, Function );
 
-  /// Set the cancellation block to \p CBB.
-  void setCancellationBlock(BasicBlock *CBB) { CancellationBlock = CBB; }
-
   /// Type used throughout for insertion points.
   using InsertPointTy = IRBuilder<>::InsertPoint;
 
+  /// Callback type for variable finalization (think destructors).
+  ///
+  /// \param CodeGenIP is the insertion point at which the finalization code
+  ///  should be placed.
+  ///
+  /// A finalize callback knows about all objects that need finalization, e.g.
+  /// destruction, when the scope of the currently generated construct is left
+  /// at the time, and location, the callback is invoked.
+  using FinalizeCallbackTy = function_ref;
+
+  struct FinalizationInfo {
+/// The finalization callback provided by the last in-flight invocation of
+/// Create for the directive of kind DK.
+FinalizeCallbackTy FiniCB;
+
+/// The directive kind of the innermost directive that has an associated
+/// region which might require finalization when it is left.
+omp::Directive DK;
+
+/// Flag to indicate if the directive is cancellable.
+bool IsCancellable;
+  };
+
+  /// Push a finalization callback on the finalization stack.
+  ///
+  /// NOTE: Temporary solution until Clang CG is gone.
+  void