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

[Alexey Bataev]

ABataev added inline comments.

================
Comment at: lib/Sema/SemaOpenMP.cpp:663
@@ -651,3 +662,3 @@
 
-bool Sema::IsOpenMPCapturedVar(VarDecl *VD) {
+bool Sema::IsOpenMPCapturedVar(VarDecl *VD, bool &PropagateCapture) {
   assert(LangOpts.OpenMP && "OpenMP is not allowed");
----------------
sfantao wrote:
> That was my initial approach, however that will not do what is required. Let me explain with an example:
> 
> ```
> int A = 0, B = 0, C = 0;
> #pragma omp threadprivate(A,B,C)
> 
> foo(){
>   ++A; ++B; ++C;
>   #pragma omp parallel copyin(A,B,C)
>   {
>     ++B;
>     #pragma omp some_other_directive
>     {
>        ++C;
>        bar(); // Uses A 
>     }
>   }
> }
> ```
> If we just change `IsOpenMPCapturedVar` as you say, this is what happens:
>  - A will not be captured so bar() will not get the right value of A
>  - C will be captured by some_other_directive, and that is not necessary, and will pose an avoidable overhead.
>  - Only B would be captured properly.
> 
> I am not saying that there is no better way of doing, but after testing several cases this is the best solution I was able to get. Let me know if you have something different to propose or if you need me to clarify anything.
Try this one:
%%% patch
Index: SemaOpenMP.cpp
===================================================================
--- SemaOpenMP.cpp      (revision 243097)
+++ SemaOpenMP.cpp      (working copy)
@@ -120,6 +120,7 @@
   /// from current directive.
   OpenMPClauseKind ClauseKindMode;
   Sema &SemaRef;
+  bool ForceCapturing;

   typedef SmallVector<SharingMapTy, 8>::reverse_iterator reverse_iterator;

@@ -130,7 +131,8 @@

 public:
   explicit DSAStackTy(Sema &S)
-      : Stack(1), ClauseKindMode(OMPC_unknown), SemaRef(S) {}
+      : Stack(1), ClauseKindMode(OMPC_unknown), SemaRef(S),
+        ForceCapturing(false) {}

   bool isClauseParsingMode() const { return ClauseKindMode != OMPC_unknown; }
   void setClauseParsingMode(OpenMPClauseKind K) { ClauseKindMode = K; }
@@ -146,6 +148,9 @@
     Stack.pop_back();
   }

+  void forceVarCapturing(bool V) { ForceCapturing = V; }
+  bool isForceVarCapturing() const { return ForceCapturing; }
+
   /// \brief If 'aligned' declaration for given variable \a D was not seen yet,
   /// add it and return NULL; otherwise return previous occurrence's expression
   /// for diagnostics.
@@ -655,7 +660,8 @@
   if (DSAStack->getCurrentDirective() != OMPD_unknown) {
     if (DSAStack->isLoopControlVariable(VD) ||
         (VD->hasLocalStorage() &&
-         isParallelOrTaskRegion(DSAStack->getCurrentDirective())))
+         isParallelOrTaskRegion(DSAStack->getCurrentDirective())) ||
+        DSAStack->isForceVarCapturing())
       return true;
     auto DVarPrivate = DSAStack->getTopDSA(VD, DSAStack->isClauseParsingMode());
     if (DVarPrivate.CKind != OMPC_unknown && isOpenMPPrivate(DVarPrivate.CKind))
@@ -1351,13 +1357,18 @@
   // This is required for proper codegen.
   for (auto *Clause : Clauses) {
     if (isOpenMPPrivate(Clause->getClauseKind()) ||
-        Clause->getClauseKind() == OMPC_copyprivate) {
+        Clause->getClauseKind() == OMPC_copyprivate ||
+        (getLangOpts().OpenMPUseTLS &&
+         getASTContext().getTargetInfo().isTLSSupported() &&
+         Clause->getClauseKind() == OMPC_copyin)) {
+      DSAStack->forceVarCapturing(Clause->getClauseKind() == OMPC_copyin);
       // Mark all variables in private list clauses as used in inner region.
       for (auto *VarRef : Clause->children()) {
         if (auto *E = cast_or_null<Expr>(VarRef)) {
           MarkDeclarationsReferencedInExpr(E);
         }
       }
+      DSAStack->forceVarCapturing(/*V=*/false);
     } else if (isParallelOrTaskRegion(DSAStack->getCurrentDirective()) &&
                Clause->getClauseKind() == OMPC_schedule) {
       // Mark all variables in private list clauses as used in inner region.
%%%


http://reviews.llvm.org/D11395