[PATCH] D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel` Directive

[Fady Ghanim via Phabricator via cfe-commits]

fghanim added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677



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

[PATCH] D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel` Directive

[Fady Ghanim via Phabricator via cfe-commits]

fghanim updated this revision to Diff 269589.
fghanim added a comment.
Herald added subscribers: aaron.ballman, sstefan1.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp


Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -1683,13 +1683,65 @@
 const CapturedStmt *CS = S.getCapturedStmt(OMPD_parallel);
 const Stmt *ParallelRegionBodyStmt = CS->getCapturedStmt();
 
-auto BodyGenCB = [ParallelRegionBodyStmt,
-  this](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-llvm::BasicBlock ) {
+OMPParallelScope Scope(*this, S);
+llvm::SmallDenseMap
+CapturedVarsInfoMap;
+
+OMPBuilderCBHelpers::GenerateOpenMPCapturedVars(*this, *CS,
+CapturedVarsInfoMap);
+
+auto BodyGenCB = [ParallelRegionBodyStmt, , this, ,
+  ](InsertPointTy AllocaIP,
+InsertPointTy CodeGenIP,
+llvm::BasicBlock ) {
   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(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();
+  PrivateScope.ForceCleanup();
+  Builder.Insert(ContTI);
 };
 
 CGCapturedStmtInfo CGSI(*CS, CR_OpenMP);


Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -1683,13 +1683,65 @@
 const CapturedStmt *CS = S.getCapturedStmt(OMPD_parallel);
 const Stmt *ParallelRegionBodyStmt = CS->getCapturedStmt();
 
-auto BodyGenCB = [ParallelRegionBodyStmt,
-  this](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-llvm::BasicBlock ) {
+OMPParallelScope Scope(*this, S);
+llvm::SmallDenseMap
+CapturedVarsInfoMap;
+
+OMPBuilderCBHelpers::GenerateOpenMPCapturedVars(*this, *CS,
+CapturedVarsInfoMap);
+
+auto BodyGenCB = [ParallelRegionBodyStmt, , this, ,
+  ](InsertPointTy AllocaIP,
+InsertPointTy CodeGenIP,
+llvm::BasicBlock ) {
   OMPBuilderCBHelpers::OutlinedRegionBodyRAII ORB(*this, AllocaIP,
   ContinuationBB);
+
+  OMPPrivateScope PrivateScope(*this);
+  llvm::BasicBlock *OMP_Entry = AllocaIP.getBlock();
+  // Emitting Copyin clause
+  

[PATCH] D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel` Directive

[Fady Ghanim via Phabricator via cfe-commits]

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

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




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:803
-  OrigVD);
-else
-  (void)CGM.getOpenMPRuntime().registerTargetFirstprivateCopy(*this,

jdoerfert wrote:
> 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).
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677



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

[PATCH] D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel` Directive

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Generally you copied the existing Clang logic, correct?




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:803
-  OrigVD);
-else
-  (void)CGM.getOpenMPRuntime().registerTargetFirstprivateCopy(*this,

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).



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:887
   *this, OrigVD);
+
   llvm::Value *V = EmitLoadOfScalar(

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



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3736
   HintInst =
-  Builder.CreateIntCast(EmitScalarExpr(Hint), CGM.Int32Ty, false);
+  Builder.CreateIntCast(EmitScalarExpr(Hint), CGM.IntPtrTy, false);
 

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79677



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

[PATCH] D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel` Directive

[Fady Ghanim via Phabricator via cfe-commits]

fghanim created this revision.
fghanim added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, guansong, yaxunl.
Herald added a project: clang.
fghanim added a parent revision: D79676: [Clang][OpenMP][OMPBuilder] Moving OMP 
allocation and cache creation code to OMPBuilderCBHelpers.

- Added support for Codegen `private` clause
- Added support for Codegen `firstprivate` Clause
- Added support for CodeGen of `copyin` Clause
- Added/moved code to support above tasks on the OMPIRBuilder


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79677

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/OpenMP/parallel_copyin_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_private_codegen.cpp

Index: clang/test/OpenMP/parallel_private_codegen.cpp
===
--- clang/test/OpenMP/parallel_private_codegen.cpp
+++ clang/test/OpenMP/parallel_private_codegen.cpp
@@ -1,8 +1,9 @@
-// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s -check-prefixes=ALL,CHECK
 // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple x86_64-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s -check-prefixes=ALL,CHECK
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -std=c++11 -DLAMBDA -triple %itanium_abi_triple -emit-llvm %s -o - | FileCheck -check-prefix=LAMBDA %s
 // RUN: %clang_cc1 -verify -fopenmp -x c++ -fblocks -DBLOCKS -triple %itanium_abi_triple -emit-llvm %s -o - | FileCheck -check-prefix=BLOCKS %s
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-enable-irbuilder -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s -check-prefixes=ALL,IRBUILDER
 
 // RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple x86_64-unknown-unknown -emit-pch -o %t %s
@@ -91,12 +92,12 @@
   }
 };
 
-// CHECK: [[SS_TY:%.+]] = type { i{{[0-9]+}}, i8
+// ALL: [[SS_TY:%.+]] = type { i{{[0-9]+}}, i8
 // LAMBDA: [[SS_TY:%.+]] = type { i{{[0-9]+}}, i8
 // BLOCKS: [[SS_TY:%.+]] = type { i{{[0-9]+}}, i8
-// CHECK: [[S_FLOAT_TY:%.+]] = type { float }
-// CHECK: [[S_INT_TY:%.+]] = type { i{{[0-9]+}} }
-// CHECK: [[SST_TY:%.+]] = type { i{{[0-9]+}} }
+// ALL: [[S_FLOAT_TY:%.+]] = type { float }
+// ALL: [[S_INT_TY:%.+]] = type { i{{[0-9]+}} }
+// ALL: [[SST_TY:%.+]] = type { i{{[0-9]+}} }
 template 
 T tmain() {
   S test;
@@ -273,63 +274,93 @@
 #endif
 }
 
-// CHECK: define i{{[0-9]+}} @main()
-// CHECK: [[TEST:%.+]] = alloca [[S_FLOAT_TY]],
-// CHECK: call {{.*}} [[S_FLOAT_TY_DEF_CONSTR:@.+]]([[S_FLOAT_TY]]* [[TEST]])
-// CHECK: call void (%{{.+}}*, i{{[0-9]+}}, void (i{{[0-9]+}}*, i{{[0-9]+}}*, ...)*, ...) @__kmpc_fork_call(%{{.+}}* @{{.+}}, i{{[0-9]+}} 0, void (i{{[0-9]+}}*, i{{[0-9]+}}*, ...)* bitcast (void (i{{[0-9]+}}*, i{{[0-9]+}}*)* [[MAIN_MICROTASK:@.+]] to void
-// CHECK: = call i{{.+}} [[TMAIN_INT:@.+]]()
-// CHECK: call void [[S_FLOAT_TY_DESTR:@.+]]([[S_FLOAT_TY]]*
-// CHECK: ret
+// ALL: define i{{[0-9]+}} @main()
+// ALL: [[TEST:%.+]] = alloca [[S_FLOAT_TY]],
+// ALL: call {{.*}} [[S_FLOAT_TY_DEF_CONSTR:@.+]]([[S_FLOAT_TY]]* [[TEST]])
+// ALL: call void (%{{.+}}*, i{{[0-9]+}}, void (i{{[0-9]+}}*, i{{[0-9]+}}*, ...)*, ...) @__kmpc_fork_call(%{{.+}}* @{{.+}}, i{{[0-9]+}} 0, void (i{{[0-9]+}}*, i{{[0-9]+}}*, ...)* bitcast (void (i{{[0-9]+}}*, i{{[0-9]+}}*)* [[MAIN_MICROTASK:@.+]] to void
+// ALL: = call i{{.+}} [[TMAIN_INT:@.+]]()
+// ALL: call void [[S_FLOAT_TY_DESTR:@.+]]([[S_FLOAT_TY]]*
+// ALL: ret
 //
-// CHECK: define internal void [[MAIN_MICROTASK]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}})
-// CHECK: [[T_VAR_PRIV:%.+]] = alloca i{{[0-9]+}},
-// CHECK: [[VEC_PRIV:%.+]] = alloca [2 x i{{[0-9]+}}],
-// CHECK: [[S_ARR_PRIV:%.+]] = alloca [2 x [[S_FLOAT_TY]]],
-// CHECK: [[VAR_PRIV:%.+]] = alloca [[S_FLOAT_TY]],
-// CHECK: [[SIVAR_PRIV:%.+]] = alloca i{{[0-9]+}},
+// ALL: define internal void [[MAIN_MICROTASK]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}})
+// ALL: [[T_VAR_PRIV:%.+]] = alloca i{{[0-9]+}},
+// ALL: [[VEC_PRIV:%.+]] = alloca [2 x i{{[0-9]+}}],
+// ALL: [[S_ARR_PRIV:%.+]] = alloca [2 x [[S_FLOAT_TY]]],
+// ALL: [[VAR_PRIV:%.+]] = alloca [[S_FLOAT_TY]],
+// ALL: [[SIVAR_PRIV:%.+]] = alloca i{{[0-9]+}},
 // CHECK: store i{{[0-9]+}}* [[GTID_ADDR]], i{{[0-9]+}}** [[GTID_ADDR_REF:%.+]]
-// CHECK-NOT: [[T_VAR_PRIV]]
-// CHECK-NOT: [[VEC_PRIV]]
-// CHECK: {{.+}}: