[PATCH] D22691: [OpenMP] Codegen for use_device_ptr clause.

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Alexey,

Thanks for the review!


================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:3410-3411
@@ +3409,4 @@
+  llvm::DenseSet<const VarDecl *> EmittedAsFirstprivate;
+  CGCapturedStmtInfo CapturesInfo(cast<CapturedStmt>(*D.getAssociatedStmt()));
+  for (const auto *C : D.getClausesOfKind<OMPUseDevicePtrClause>()) {
+    auto OrigVarIt = C->varlist_begin();
----------------
ABataev wrote:
> EmitOMPUseDevicePtrClause() should emit the code for single clause. The outer loop must be used in directive emission function.
Ok ,makes sense, Doing as you say.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:3477-3495
@@ +3476,21 @@
+  // Codegen with device pointer privatization.
+  auto &&PvtCodeGen = [&S, &Info](CodeGenFunction &CGF, PrePostActionTy &) {
+    auto &&CodeGen = [&S, &Info](CodeGenFunction &CGF, PrePostActionTy &) {
+      OMPPrivateScope PrivateScope(CGF);
+      CGF.EmitOMPUseDevicePtrClause(S, PrivateScope, Info.CaptureDeviceAddrMap);
+      (void)PrivateScope.Privatize();
+      CGF.EmitStmt(
+          cast<CapturedStmt>(S.getAssociatedStmt())->getCapturedStmt());
+    };
+    // Notwithstanding the body of the region is emitted as inlined directive,
+    // we don't use an inline scope as changes in the references inside the
+    // region are expected to be visible outside, so we do not privative them.
+    OMPLexicalScope Scope(CGF, S);
+    CGF.CGM.getOpenMPRuntime().emitInlinedDirective(CGF, OMPD_target_data,
+                                                    CodeGen);
+  };
+
+  // Codegen with no device pointer privatization.
+  auto &&NoPvtCodeGen = [&S, &Info](CodeGenFunction &CGF, PrePostActionTy &) {
+    auto &&CodeGen = [&S, &Info](CodeGenFunction &CGF, PrePostActionTy &) {
+      CGF.EmitStmt(
----------------
ABataev wrote:
> PvtCodeGen and NoPvtCodeGen are pretty similar. Could you merge them somehow into the one? Maybe some custom PrePostAction could be used here?
I replaced this code by a single region codegen controlling the privatization with a flag and a pre-action. Let me know if this what you had in mind.

================
Comment at: lib/Sema/SemaOpenMP.cpp:11858
@@ +11857,3 @@
+    // similar properties of a first private variable.
+    DSAStack->addDSA(D, RefExpr->IgnoreParens(), OMPC_firstprivate, Ref);
+
----------------
ABataev wrote:
> Are the restrictions for firstprivates applied also? What if inner directive has some clauses that cannot be used with variables previously marked as firstprivates, but can be used with variables, used in use_device_ptr?
I was revisiting the spec and I couldn't find any conflict with the `firstprivate` restrictions. There are conflicts for variables used in a firstprivate clause that are also used in clauses of enclosing worksharing constructs but not the other way around. Note that we are using the firstprivate attribute but do not do all the firstprivate checks, so the conflicts would only arise from other enclosed constructs, not other enclosing constructs. Let me know if you have any specific case in mind or if you'd like me to do this differently.  


https://reviews.llvm.org/D22691