[PATCH] D11395: [OpenMP] Add capture for threadprivate variables used in copyin clause

[Samuel Antao]

sfantao added inline comments.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:230-246
@@ -229,7 +229,19 @@
       if (CopiedVars.insert(VD->getCanonicalDecl()).second) {
-        // Get the address of the master variable.
-        auto *MasterAddr = VD->isStaticLocal()
-                               ? CGM.getStaticLocalDeclAddress(VD)
-                               : CGM.GetAddrOfGlobal(VD);
+
+        // Get the address of the master variable. If we are emitting code with
+        // TLS support, the address is passed from the master as field in the
+        // captured declaration.
+        llvm::Value *MasterAddr;
+        if (getLangOpts().OpenMPUseTLS &&
+            getContext().getTargetInfo().isTLSSupported()) {
+          assert(CapturedStmtInfo->lookup(VD) &&
+                 "Copyin threadprivates should have been captured!");
+          DeclRefExpr DRE(const_cast<VarDecl *>(VD), true, (*IRef)->getType(),
+                          VK_LValue, (*IRef)->getExprLoc());
+          MasterAddr = EmitLValue(&DRE).getAddress();
+        } else {
+          MasterAddr = VD->isStaticLocal() ? CGM.getStaticLocalDeclAddress(VD)
+                                           : CGM.GetAddrOfGlobal(VD);
+        }
         // Get the address of the threadprivate variable.
         auto *PrivateAddr = EmitLValue(*IRef).getAddress();
----------------
Done with a small modification: the implementation always expect the `CapturedStmtInfo` to contain a valid lookup. 

================
Comment at: lib/Sema/SemaOpenMP.cpp:160-667
@@ -159,4 +159,8 @@
 
+  /// \brief Check if the specified variable is a 'copyin' variable for current
+  /// region.
+  bool isCopyinVariable(VarDecl *D);
+
   /// \brief Adds explicit data sharing attribute to the specified declaration.
   void addDSA(VarDecl *D, DeclRefExpr *E, OpenMPClauseKind A);
 

@@ -412,6 +416,12 @@
   return Stack.back().LCVSet.count(D) > 0;
 }
 
+bool DSAStackTy::isCopyinVariable(VarDecl *D){
+  assert(Stack.size() > 1 && "Data-sharing attributes stack is empty");
+  D = D->getCanonicalDecl();
+  DSAInfo I = Stack.back().SharingMap.lookup(D);
+  return I.Attributes == OMPC_copyin;
+}
 void DSAStackTy::addDSA(VarDecl *D, DeclRefExpr *E, OpenMPClauseKind A) {
   D = D->getCanonicalDecl();
   if (A == OMPC_threadprivate) {

@@ -653,4 +663,6 @@
   assert(LangOpts.OpenMP && "OpenMP is not allowed");
   VD = VD->getCanonicalDecl();
   if (DSAStack->getCurrentDirective() != OMPD_unknown) {
+    if (DSAStack->isCopyinVariable(VD) && !DSAStack->isClauseParsingMode())
+      return true;
     if (DSAStack->isLoopControlVariable(VD) ||
----------------
ABataev wrote:
> ABataev wrote:
> > I'd better write a new function isOpenMPPrivateOrCopyin() function and used it instead of isOpenMPPrivate() in Sema::IsOpenMPCapturedVar(). Besides, note, that copyin variables must be captured only if TLS mode is on, if we're using runtime calls we don't need to capture threadprivates.
> If you want to capture copyin variable only in closely nested OpenMP region, it is enough just to make next changes in Sema::IsOpenMPCapturedVar():
> ...
> auto DVarPrivate = DSAStack->getTopDSA(VD, DSAStack->isClauseParsingMode());
> if (DVarPrivate.CKind != OMPC_unknown && (isOpenMPPrivate(DVarPrivate.CKind) || (getLangOpts().OpenMPTLS && DVarPrivate.CKind == OMPC_copyin))
> ...
The TLS check is now implemented in `IsOpenMPCapturedVar`. However I am not combining private with copyin because the action takes is slightly different.

================
Comment at: lib/Sema/SemaOpenMP.cpp:160-667
@@ -159,4 +159,8 @@
 
+  /// \brief Check if the specified variable is a 'copyin' variable for current
+  /// region.
+  bool isCopyinVariable(VarDecl *D);
+
   /// \brief Adds explicit data sharing attribute to the specified declaration.
   void addDSA(VarDecl *D, DeclRefExpr *E, OpenMPClauseKind A);
 

@@ -412,6 +416,12 @@
   return Stack.back().LCVSet.count(D) > 0;
 }
 
+bool DSAStackTy::isCopyinVariable(VarDecl *D){
+  assert(Stack.size() > 1 && "Data-sharing attributes stack is empty");
+  D = D->getCanonicalDecl();
+  DSAInfo I = Stack.back().SharingMap.lookup(D);
+  return I.Attributes == OMPC_copyin;
+}
 void DSAStackTy::addDSA(VarDecl *D, DeclRefExpr *E, OpenMPClauseKind A) {
   D = D->getCanonicalDecl();
   if (A == OMPC_threadprivate) {

@@ -653,4 +663,6 @@
   assert(LangOpts.OpenMP && "OpenMP is not allowed");
   VD = VD->getCanonicalDecl();
   if (DSAStack->getCurrentDirective() != OMPD_unknown) {
+    if (DSAStack->isCopyinVariable(VD) && !DSAStack->isClauseParsingMode())
+      return true;
     if (DSAStack->isLoopControlVariable(VD) ||
----------------
sfantao wrote:
> ABataev wrote:
> > ABataev wrote:
> > > I'd better write a new function isOpenMPPrivateOrCopyin() function and used it instead of isOpenMPPrivate() in Sema::IsOpenMPCapturedVar(). Besides, note, that copyin variables must be captured only if TLS mode is on, if we're using runtime calls we don't need to capture threadprivates.
> > If you want to capture copyin variable only in closely nested OpenMP region, it is enough just to make next changes in Sema::IsOpenMPCapturedVar():
> > ...
> > auto DVarPrivate = DSAStack->getTopDSA(VD, DSAStack->isClauseParsingMode());
> > if (DVarPrivate.CKind != OMPC_unknown && (isOpenMPPrivate(DVarPrivate.CKind) || (getLangOpts().OpenMPTLS && DVarPrivate.CKind == OMPC_copyin))
> > ...
> The TLS check is now implemented in `IsOpenMPCapturedVar`. However I am not combining private with copyin because the action takes is slightly different.
Capturing in the closely nested region is not sufficient: declarations have to be captured even if no uses are found. Please, take a look at the last patch and let me know your thoughts.


http://reviews.llvm.org/D11395