[Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

[fghanim]

Summary

Adding IMplementation of private, firstprivate, copyin clauses to
the OMPBuilder implementation of omp parallel in clang

[jdoerfert]

Generally you copied the existing Clang logic, correct?

clang/lib/CodeGen/CGStmtOpenMP.cpp

796–797

Wasn't this part of D79675?

(btw I tried to comprehend why this is needed and it is on my list for things we replace eventually).

885

Please undo as it doesn't seem to change anything.

3449

These 4 lines can go in on their own, LGTM on them.

[fghanim]

In D79677#2032942, @jdoerfert wrote:

Generally you copied the existing Clang logic, correct?

Well, Yes and no. I tried to keep as much as I can of the original implementation, however, some required more extensive changes.

The things added to emitparalleldirective are all new
Almost the latter half of the OMPBuilder version of emitfirstprivateclause
somethings in the the OMPBuilder version of emitcopyinclause
The rest had minor or smaller changes, but generally the same
the lit tests had some changes

clang/lib/CodeGen/CGStmtOpenMP.cpp

796–797

ignore - I originally wanted to use the original emitfirstprivate, before I had to make some changes. This is remaining from that code. The same comment / todo is in OMPBuilder specific version below.

I also, removed this from D79676

[fghanim]

• rebase
• splitting patch into 4 ( this, D81482 , D81483 , D81484 )
• addressing reviewer's comments

[fghanim]

@jdoerfert Please suggest reviewer's for this, and I will add them to other clang related patches

[fghanim]

ping - please suggest reviewers I can add to review the clang side of things?

[kiranchandramohan]

Thanks @fghanim for this patch. I will get to this on Friday if you can add me as a reviewer.

[jdoerfert]

@kiranchandramohan You can add yourself and others ;)

Can we test this?

[kiranchandramohan]

Is the ordering of code generation for clauses important?
copyin -> firstprivate -> barrier -> private

clang/lib/CodeGen/CGStmtOpenMP.cpp

1673

Should this change in this patch?

1729–1736

Nit: What do these three cases correspond to? A comment might be useful.

1743

Not a comment about this patch: While the context makes it clear, the name does not suggest that this function is emitting something.

[fghanim]

In D79677#2119019, @kiranchandramohan wrote:

Is the ordering of code generation for clauses important?
copyin -> firstprivate -> barrier -> private

if we emitted a copyin, then prob. yes. otherwise no.

clang/lib/CodeGen/CGStmtOpenMP.cpp

1673

No, still used for shared variables.

but the todo should go away.

1729–1736

Just checks to see what to do with the terminator of the entry block.

I'll add a comment in an update

1743

you mean ForceClean()?
it forces the emission of cleanup code - instead of just as part of the Privatescope Dctor.

[kiranchandramohan]

What is the plan for this patch?

[fghanim]

In D79677#2248197, @kiranchandramohan wrote:

What is the plan for this patch?

To commit it ... eventually :)
Once it (and the rest in the series) get reviewed. As it stands, I cannot commit this patch without the rest in the series, and I cannot find reviewers for the other patches.

[lebedev.ri]

Tests missing

[fghanim]

In D79677#2248663, @lebedev.ri wrote:

Tests missing

I am not sure what to test here that isn't tested elsewhere in the series. This patch is the last in a series, and it represents the "usage" of the functionality added by the other patches. Each of the other patches has its test as a part of it.

clang/lib/CodeGen/CGStmtOpenMP.cpp

 //===--- CGStmtOpenMP.cpp - Emit LLVM Code from Statements ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
for (const Expr *IInit : C->private_copies()) {
         ++IRef;
         ++InitsRef;
         continue;
       }
       // Do not emit copy for firstprivate constant variables in target regions,
       // captured by reference.
       if (DeviceConstTarget && OrigVD->getType().isConstant(getContext()) &&
           FD && FD->getType()->isReferenceType() &&
           (!VD || !VD->hasAttr<OMPAllocateDeclAttr>())) {
         (void)CGM.getOpenMPRuntime().registerTargetFirstprivateCopy(*this,
                                                                     OrigVD);
         ++IRef;
         ++InitsRef;
         continue;
       }
       FirstprivateIsLastprivate =
           FirstprivateIsLastprivate || ThisFirstprivateIsLastprivate;
       if (EmittedAsFirstprivate.insert(OrigVD->getCanonicalDecl()).second) {
for (const Expr *IInit : C->private_copies()) {
                 LocalDeclMap.erase(VDInit);
                 if (ThisFirstprivateIsLastprivate &&
                     Lastprivates[OrigVD->getCanonicalDecl()] ==
                         OMPC_LASTPRIVATE_conditional) {
                   // Create/init special variable for lastprivate conditionals.
                   Address VDAddr =
                       CGM.getOpenMPRuntime().emitLastprivateConditionalInit(
                           *this, OrigVD);
                   llvm::Value *V = EmitLoadOfScalar(
                       MakeAddrLValue(GetAddrOfLocalVar(VD), (*IRef)->getType(),
                                      AlignmentSource::Decl),
                       (*IRef)->getExprLoc());
                   EmitStoreOfScalar(V,
                                     MakeAddrLValue(VDAddr, (*IRef)->getType(),
                                                    AlignmentSource::Decl));
                   LocalDeclMap.erase(VD);
                   setAddrOfLocalVar(VD, VDAddr);
if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) {
     // thus calls destructors etc.
     auto FiniCB = [this](InsertPointTy IP) {
       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,
+    OMPParallelScope Scope(*this, S);
+    llvm::SmallDenseMap<const VarDecl *, OMPBuilderCBHelpers::CapturedVarInfo>
+        CapturedVarsInfoMap;
+
+    OMPBuilderCBHelpers::GenerateOpenMPCapturedVars(*this, *CS,
+                                                    CapturedVarsInfoMap);
+
+    auto BodyGenCB = [ParallelRegionBodyStmt, &S, this, &OMPBuilder,
+                      &CapturedVarsInfoMap](InsertPointTy AllocaIP,
+                                            InsertPointTy CodeGenIP,
                                             llvm::BasicBlock &ContinuationBB) {
       OMPBuilderCBHelpers::OutlinedRegionBodyRAII ORB(*this, AllocaIP,
                                                       ContinuationBB);
+
+      OMPPrivateScope PrivateScope(*this);
+      llvm::BasicBlock *OMP_Entry = AllocaIP.getBlock();
+      // Emitting Copyin clause
+      Builder.SetInsertPoint(OMP_Entry->getTerminator());
+      bool Copyins =
+          OMPBuilderCBHelpers::EmitOMPCopyinClause(*this, S, AllocaIP);
+
+      // restoring alloca insertion point to entry block since it moved while
+      // emitting 'copyin' blocks
+      AllocaInsertPt = OMP_Entry->getFirstNonPHIOrDbgOrLifetime();
+      llvm::BranchInst *EntryBI =
+          cast<llvm::BranchInst>(OMP_Entry->getTerminator());
+      EntryBI->removeFromParent();
+
+      if (Builder.GetInsertBlock() == OMP_Entry)
+        Builder.SetInsertPoint(OMP_Entry);
+      OMPBuilderCBHelpers::EmitOMPFirstprivateClause(*this, S, PrivateScope,
+                                                     CapturedVarsInfoMap);
+      if (Copyins) {
+        // Emit implicit barrier to synchronize threads and avoid data races on
+        // propagation master's thread values of threadprivate variables to
+        // local instances of that variables of all other implicit threads.
+        OMPBuilder->CreateBarrier(Builder, OMPD_barrier, /*EmitChecks=*/false,
+                                  /*ForceSimpleCall=*/true);
+      }
+
+      EmitOMPPrivateClause(S, PrivateScope);
+      (void)PrivateScope.Privatize();
+
+      if (!OMP_Entry->getTerminator()) {
+        OMP_Entry->getInstList().push_back(EntryBI);
+      } else if (Builder.GetInsertBlock()->getTerminator()) {
+        EntryBI->dropAllReferences();
+        EntryBI->deleteValue();
+      } else {
+        Builder.Insert(EntryBI);
+      }
+
       OMPBuilderCBHelpers::EmitOMPRegionBody(*this, ParallelRegionBodyStmt,
                                              CodeGenIP, ContinuationBB);
+      llvm::Instruction *ContTI = ContinuationBB.getTerminator();
+      ContTI->removeFromParent();
+      Builder.SetInsertPoint(&ContinuationBB);
+      PrivateScope.ForceCleanup();
+      Builder.Insert(ContTI);
     };
 
     CGCapturedStmtInfo CGSI(*CS, CR_OpenMP);
     CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(*this, &CGSI);
     Builder.restoreIP(OMPBuilder->CreateParallel(Builder, BodyGenCB, PrivCB,
                                                  FiniCB, IfCond, NumThreads,
                                                  ProcBind, S.hasCancel()));
     return;
if (const auto *HintClause = S.getSingleClause<OMPHintClause>())
       Hint = HintClause->getHint();
 
     // 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);
 
     auto FiniCB = [this](InsertPointTy IP) {
       OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
     };
 
     auto BodyGenCB = [CriticalRegionBodyStmt, this](InsertPointTy AllocaIP,
                                                     InsertPointTy CodeGenIP,
                                                     llvm::BasicBlock &FiniBB) {