[OpenMP][OMPIRBuilder] Introducing the `OMPBuilderCBHelpers` helper class

[fghanim]

Summary

This patch introduces a new helper class OMPBuilderCBHelpers,
which will contain all reusable C/C++ language specific function-
alities required by the OMPIRBuilder.

Initially, this helper class contains the body and finalization
codegen functionalities implemented using callbacks which were
moved here for reusability among the different directives
implemented in the OMPIRBuilder, along with RAIIs for preserving
state prior to emitting outlined and/or inlined OpenMP regions.

In the future this helper class will also contain all the different
call backs required by OpenMP clauses/variable privatization.

[jdoerfert]

This is great, thanks a lot! I only have two comments where I am not sure I understand the code/change.

clang/lib/CodeGen/CGStmtOpenMP.cpp

3132–3133

These TODOs are now obsolete ;)

clang/lib/CodeGen/CodeGenFunction.h

354

Don't we have to set/reset the CGF.ReturnBlock ? If not, why do we need FinilizationBlock here?

clang/test/OpenMP/parallel_codegen.cpp

143 ↗

(On Diff #244443)

Do you know why this changed? Is this variable used in the parallel region?

[fghanim]

Fixed bug where variables where still being allocated in original function entry block

[fghanim]

Regarding the third comment (which was removed for some reason), This new update should fix that

clang/lib/CodeGen/CGStmtOpenMP.cpp

3132–3133

We need to split our patches to keep them small :p

clang/lib/CodeGen/CodeGenFunction.h

354

Yes, and we still do for outlined regions (check OutlinedRegionBodyRAII ctor above).

This is for inlined regions, and those shouldn't change the CGF.ReturnBlock. It should re-use the CGF.ReturnBlock for the enclosing outlined region.
The only reason I kept it, is because CGF.getJumpDestInCurrentScope() was updating a counter that seemed to me to relate to EHStack, and I didn't want to remove it without investigating first.

[jdoerfert]

LGTM assuming the comment below is addressed.

clang/lib/CodeGen/CodeGenFunction.h

354

I see. Maybe remove the FinilizationBlock member though. Call getJumpDestInCurrentScope and add a comment (TODO) that explains this, e.g., says it has to be investigated further.

[fghanim]

addressing review comments - Adding a comment to explain minor change.

[fghanim]

Marking a call void

[fghanim]

I am done updating this patch.
I still don't have commit access, I'd appreciate it if you'd commit this for me when you get a chance.

clang/lib/CodeGen/CGStmtOpenMP.cpp

if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) {
     if (const auto *ProcBindClause = S.getSingleClause<OMPProcBindClause>())
       ProcBind = ProcBindClause->getProcBindKind();
 
     using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
 
     // The cleanup callback that finalizes all variabels at the given location,
     // thus calls destructors etc.
     auto FiniCB = [this](InsertPointTy IP) {
-      CGBuilderTy::InsertPointGuard IPG(Builder);
-      assert(IP.getBlock()->end() != IP.getPoint() &&
-             "OpenMP IR Builder should cause terminated block!");
-      llvm::BasicBlock *IPBB = IP.getBlock();
-      llvm::BasicBlock *DestBB = IPBB->splitBasicBlock(IP.getPoint());
-      IPBB->getTerminator()->eraseFromParent();
-      Builder.SetInsertPoint(IPBB);
-      CodeGenFunction::JumpDest Dest = getJumpDestInCurrentScope(DestBB);
-      EmitBranchThroughCleanup(Dest);
+      OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
     };
 
     // Privatization callback that performs appropriate action for
     // shared/private/firstprivate/lastprivate/copyin/... variables.
     //
     // TODO: This defaults to shared right now.
     auto PrivCB = [](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
                      llvm::Value &Val, llvm::Value *&ReplVal) {
       // The next line is appropriate only for variables (Val) with the
       // data-sharing attribute "shared".
       ReplVal = &Val;
 
       return CodeGenIP;
     };
 
     const CapturedStmt *CS = S.getCapturedStmt(OMPD_parallel);
     const Stmt *ParallelRegionBodyStmt = CS->getCapturedStmt();
 
     auto BodyGenCB = [ParallelRegionBodyStmt,
                       this](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
                             llvm::BasicBlock &ContinuationBB) {
-      auto OldAllocaIP = AllocaInsertPt;
-      AllocaInsertPt = &*AllocaIP.getPoint();
-
-      auto OldReturnBlock = ReturnBlock;
-      ReturnBlock = getJumpDestInCurrentScope(&ContinuationBB);
-
-      llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
-      CodeGenIPBB->splitBasicBlock(CodeGenIP.getPoint());
-      llvm::Instruction *CodeGenIPBBTI = CodeGenIPBB->getTerminator();
-      CodeGenIPBBTI->removeFromParent();
-
-      Builder.SetInsertPoint(CodeGenIPBB);
-
-      EmitStmt(ParallelRegionBodyStmt);
-
-      Builder.Insert(CodeGenIPBBTI);
-
-      AllocaInsertPt = OldAllocaIP;
-      ReturnBlock = OldReturnBlock;
+      OMPBuilderCBHelpers::OutlinedRegionBodyRAII ORB(*this, AllocaIP,
+                                                      ContinuationBB);
+      OMPBuilderCBHelpers::EmitOMPRegionBody(*this, ParallelRegionBodyStmt,
+                                             CodeGenIP, ContinuationBB);
     };
 
     CGCapturedStmtInfo CGSI(*CS, CR_OpenMP);
     CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(*this, &CGSI);
     Builder.restoreIP(OMPBuilder->CreateParallel(Builder, BodyGenCB, PrivCB,
                                                  FiniCB, IfCond, NumThreads,
                                                  ProcBind, S.hasCancel()));
     return;

 
 void CodeGenFunction::EmitOMPMasterDirective(const OMPMasterDirective &S) {
   if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) {
     using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
 
     const CapturedStmt *CS = S.getInnermostCapturedStmt();
     const Stmt *MasterRegionBodyStmt = CS->getCapturedStmt();
 
-    // TODO: Replace with a generic helper function for finalization
     auto FiniCB = [this](InsertPointTy IP) {
-      CGBuilderTy::InsertPointGuard IPG(Builder);
-      assert(IP.getBlock()->end() != IP.getPoint() &&
-             "OpenMP IR Builder should cause terminated block!");
-
-      llvm::BasicBlock *IPBB = IP.getBlock();
-      llvm::BasicBlock *DestBB = IPBB->getUniqueSuccessor();
-      assert(DestBB && "Finalization block should have one successor!");
-
-      // erase and replace with cleanup branch.
-      IPBB->getTerminator()->eraseFromParent();
-      Builder.SetInsertPoint(IPBB);
-      CodeGenFunction::JumpDest Dest = getJumpDestInCurrentScope(DestBB);
-      EmitBranchThroughCleanup(Dest);
+      OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
     };
 
-    // TODO: Replace with a generic helper function for emitting body
     auto BodyGenCB = [MasterRegionBodyStmt, this](InsertPointTy AllocaIP,
                                                   InsertPointTy CodeGenIP,
                                                   llvm::BasicBlock &FiniBB) {
-      // Alloca insertion block should be in the entry block of the containing
-      // function So it expects an empty AllocaIP in which case will reuse the
-      // old alloca insertion point, or a new AllocaIP in the same block as the
-      // old one
-      assert((!AllocaIP.isSet() ||
-              AllocaInsertPt->getParent() == AllocaIP.getBlock()) &&
-             "Insertion point should be in the entry block of containing "
-             "function!");
-      auto OldAllocaIP = AllocaInsertPt;
-      if (AllocaIP.isSet())
-        AllocaInsertPt = &*AllocaIP.getPoint();
-      auto OldReturnBlock = ReturnBlock;
-      ReturnBlock = getJumpDestInCurrentScope(&FiniBB);
-
-      llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
-      if (llvm::Instruction *CodeGenIPBBTI = CodeGenIPBB->getTerminator())
-        CodeGenIPBBTI->eraseFromParent();
-
-      Builder.SetInsertPoint(CodeGenIPBB);
-
-      EmitStmt(MasterRegionBodyStmt);
-
-      if (Builder.saveIP().isSet())
-        Builder.CreateBr(&FiniBB);
-
-      AllocaInsertPt = OldAllocaIP;
-      ReturnBlock = OldReturnBlock;
+      OMPBuilderCBHelpers::InlinedRegionBodyRAII IRB(*this, AllocaIP, FiniBB);
+      OMPBuilderCBHelpers::EmitOMPRegionBody(*this, MasterRegionBodyStmt,
+                                             CodeGenIP, FiniBB);
     };
+
     CGCapturedStmtInfo CGSI(*CS, CR_OpenMP);
     CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(*this, &CGSI);
     Builder.restoreIP(OMPBuilder->CreateMaster(Builder, BodyGenCB, FiniCB));
 
     return;
   }
   OMPLexicalScope Scope(*this, S, OMPD_unknown);
   emitMaster(*this, S);
if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) {
     // TODO: This is slightly different from what's currently being done in
     // clang. Fix the Int32Ty to IntPtrTy (pointer width size) when everything
     // about typing is final.
     llvm::Value *HintInst = nullptr;
     if (Hint)
       HintInst =
           Builder.CreateIntCast(EmitScalarExpr(Hint), CGM.Int32Ty, false);
 
-    // TODO: Replace with a generic helper function for finalization
     auto FiniCB = [this](InsertPointTy IP) {
-      CGBuilderTy::InsertPointGuard IPG(Builder);
-      assert(IP.getBlock()->end() != IP.getPoint() &&
-             "OpenMP IR Builder should cause terminated block!");
-      llvm::BasicBlock *IPBB = IP.getBlock();
-      llvm::BasicBlock *DestBB = IPBB->getUniqueSuccessor();
-      assert(DestBB && "Finalization block should have one successor!");
-
-      // erase and replace with cleanup branch.
-      IPBB->getTerminator()->eraseFromParent();
-      Builder.SetInsertPoint(IPBB);
-      CodeGenFunction::JumpDest Dest = getJumpDestInCurrentScope(DestBB);
-      EmitBranchThroughCleanup(Dest);
+      OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
     };
 
-    // TODO: Replace with a generic helper function for emitting body
     auto BodyGenCB = [CriticalRegionBodyStmt, this](InsertPointTy AllocaIP,
                                                     InsertPointTy CodeGenIP,
                                                     llvm::BasicBlock &FiniBB) {
-      // Alloca insertion block should be in the entry block of the containing
-      // function So it expects an empty AllocaIP in which case will reuse the
-      // old alloca insertion point, or a new AllocaIP in the same block as the
-      // old one
-      assert((!AllocaIP.isSet() ||
-              AllocaInsertPt->getParent() == AllocaIP.getBlock()) &&
-             "Insertion point should be in the entry block of containing "
-             "function!");
-      auto OldAllocaIP = AllocaInsertPt;
-      if (AllocaIP.isSet())
-        AllocaInsertPt = &*AllocaIP.getPoint();
-      auto OldReturnBlock = ReturnBlock;
-      ReturnBlock = getJumpDestInCurrentScope(&FiniBB);
-
-      llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
-      if (llvm::Instruction *CodeGenIPBBTI = CodeGenIPBB->getTerminator())
-        CodeGenIPBBTI->eraseFromParent();
-
-      Builder.SetInsertPoint(CodeGenIPBB);
-
-      EmitStmt(CriticalRegionBodyStmt);
-
-      if (Builder.saveIP().isSet())
-        Builder.CreateBr(&FiniBB);
-
-      AllocaInsertPt = OldAllocaIP;
-      ReturnBlock = OldReturnBlock;
+      OMPBuilderCBHelpers::InlinedRegionBodyRAII IRB(*this, AllocaIP, FiniBB);
+      OMPBuilderCBHelpers::EmitOMPRegionBody(*this, CriticalRegionBodyStmt,
+                                             CodeGenIP, FiniBB);
     };
 
     CGCapturedStmtInfo CGSI(*CS, CR_OpenMP);
     CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(*this, &CGSI);
     Builder.restoreIP(OMPBuilder->CreateCritical(
         Builder, BodyGenCB, FiniCB, S.getDirectiveName().getAsString(),
         HintInst));
 

clang/lib/CodeGen/CodeGenFunction.h

 #include "clang/Basic/CapturedStmt.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/OpenMPKinds.h"
 #include "clang/Basic/TargetInfo.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Utils/SanitizerStats.h"
 
 namespace llvm {
 class BasicBlock;
 class LLVMContext;
 class MDNode;
struct JumpDest {
     }
 
   private:
     llvm::BasicBlock *Block;
     EHScopeStack::stable_iterator ScopeDepth;
     unsigned Index;
   };
 
+  // Helper class for the OpenMP IR Builder. Allows reusability of code used for
+  // region body, and finalization codegen callbacks. This will class will also
+  // contain privatization functions used by the privatization call backs
+  struct OMPBuilderCBHelpers {
+
+    using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
+
+    /// Emit the Finalization for an OMP region
+    /// \param CGF  The Codegen function this belongs to
+    /// \param IP Insertion point for generating the finalization code.
+    static void FinalizeOMPRegion(CodeGenFunction &CGF, InsertPointTy IP) {
+      CGBuilderTy::InsertPointGuard IPG(CGF.Builder);
+      assert(IP.getBlock()->end() != IP.getPoint() &&
+             "OpenMP IR Builder should cause terminated block!");
+
+      llvm::BasicBlock *IPBB = IP.getBlock();
+      llvm::BasicBlock *DestBB = IPBB->getUniqueSuccessor();
+      assert(DestBB && "Finalization block should have one successor!");
+
+      // erase and replace with cleanup branch.
+      IPBB->getTerminator()->eraseFromParent();
+      CGF.Builder.SetInsertPoint(IPBB);
+      CodeGenFunction::JumpDest Dest = CGF.getJumpDestInCurrentScope(DestBB);
+      CGF.EmitBranchThroughCleanup(Dest);
+    }
+
+    /// Emit the body of an OMP region
+    /// \param CGF  The Codegen function this belongs to
+    /// \param RegionBodyStmt The body statement for the OpenMP region being
+    ///        generated
+    /// \param CodeGenIP  Insertion point for generating the body code.
+    /// \param FiniBB The finalization basic block
+    static void EmitOMPRegionBody(CodeGenFunction &CGF,
+                                  const Stmt *RegionBodyStmt,
+                                  InsertPointTy CodeGenIP,
+                                  llvm::BasicBlock &FiniBB) {
+      llvm::BasicBlock *CodeGenIPBB = CodeGenIP.getBlock();
+      if (llvm::Instruction *CodeGenIPBBTI = CodeGenIPBB->getTerminator())
+        CodeGenIPBBTI->eraseFromParent();
+
+      CGF.Builder.SetInsertPoint(CodeGenIPBB);
+
+      CGF.EmitStmt(RegionBodyStmt);
+
+      if (CGF.Builder.saveIP().isSet())
+        CGF.Builder.CreateBr(&FiniBB);
+    }
+
+    /// RAII for preserving necessary info during Outlined region body codegen.
+    class OutlinedRegionBodyRAII {
+
+      llvm::AssertingVH<llvm::Instruction> OldAllocaIP;
+      CodeGenFunction::JumpDest OldReturnBlock;
+      CodeGenFunction &CGF;
+
+    public:
+      OutlinedRegionBodyRAII(CodeGenFunction &cgf, InsertPointTy &AllocaIP,
+                             llvm::BasicBlock &RetBB)
+          : CGF(cgf) {
+        assert(AllocaIP.isSet() &&
+               "Must specify Insertion point for allocas of outlined function");
+        OldAllocaIP = CGF.AllocaInsertPt;
+        CGF.AllocaInsertPt = &*AllocaIP.getPoint();
+
+        OldReturnBlock = CGF.ReturnBlock;
+        CGF.ReturnBlock = CGF.getJumpDestInCurrentScope(&RetBB);
+      }
+
+      ~OutlinedRegionBodyRAII() {
+        CGF.AllocaInsertPt = OldAllocaIP;
+        CGF.ReturnBlock = OldReturnBlock;
+      }
+    };
+
+    /// RAII for preserving necessary info during inlined region body codegen.
+    class InlinedRegionBodyRAII {
+
+      llvm::AssertingVH<llvm::Instruction> OldAllocaIP;
+      CodeGenFunction &CGF;
+
+    public:
+      InlinedRegionBodyRAII(CodeGenFunction &cgf, InsertPointTy &AllocaIP,
+                            llvm::BasicBlock &FiniBB)
+          : CGF(cgf) {
+        // Alloca insertion block should be in the entry block of the containing
+        // function so it expects an empty AllocaIP in which case will reuse the
+        // old alloca insertion point, or a new AllocaIP in the same block as
+        // the old one
+        assert((!AllocaIP.isSet() ||
+                CGF.AllocaInsertPt->getParent() == AllocaIP.getBlock()) &&
+               "Insertion point should be in the entry block of containing "
+               "function!");
+        OldAllocaIP = CGF.AllocaInsertPt;
+        if (AllocaIP.isSet())
+          CGF.AllocaInsertPt = &*AllocaIP.getPoint();
+
+        // TODO: Remove the call, after making sure the counter is not used by
+        //       the EHStack.
+        // Since this is an inlined region, it should not modify the
+        // ReturnBlock, and should reuse the one for the enclosing outlined
+        // region. So, the JumpDest being return by the function is discarded
+        (void)CGF.getJumpDestInCurrentScope(&FiniBB);
+      }
+
+      ~InlinedRegionBodyRAII() { CGF.AllocaInsertPt = OldAllocaIP; }
+    };
+  };
+
   CodeGenModule &CGM;  // Per-module state.
   const TargetInfo &Target;
 
   typedef std::pair<llvm::Value *, llvm::Value *> ComplexPairTy;
   LoopInfoStack LoopStack;
   CGBuilderTy Builder;
 
   // Stores variables for which we can't generate correct lifetime markers

clang/test/OpenMP/cancel_codegen.cpp

 // The barrier directive should now call __kmpc_cancel_barrier
 // IRBUILDER: call i32 @__kmpc_cancel_barrier(%struct.ident_t*
 // IRBUILDER: br label
 // IRBUILDER: [[THEN]]
 // IRBUILDER: [[RES:%.+]] = call i32 @__kmpc_cancel(%struct.ident_t* {{[^,]+}}, i32 {{[^,]+}}, i32 1)
 // IRBUILDER: [[CMP:%.+]] = icmp eq i32 [[RES]], 0
 // IRBUILDER: br i1 [[CMP]], label %[[CONTINUE:[^,].+]], label %[[EXIT:.+]]
 // IRBUILDER: [[EXIT]]
-// IRBUILDER: br label %[[EXIT2:.+]]
-// IRBUILDER: [[EXIT2]]
-// IRBUILDER: br label %[[RETURN]]
+// IRBUILDER: br label %[[RETURN:.+]]
 // IRBUILDER: [[CONTINUE]]
 // IRBUILDER: br label %[[ELSE:.+]]
 
 #endif