[PATCH] D18110: [OpenMP] Fix SEMA bug in the capture of global variables in template functions.

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Alexey,

Thanks for the review.


================
Comment at: lib/Sema/SemaOpenMP.cpp:816-822
@@ -801,6 +815,9 @@
+
+  // A DSA refers to this captured region if the parent contexts match.
+  auto *ParentContext = RSI->TheCapturedDecl->getParent();
   for (auto I = Stack.rbegin(), EE = Stack.rend(); I != EE; ++I)
-    if (I->CurScope == S)
+    if (I->ParentDeclContext == ParentContext)
       return I->Directive;
   return OMPD_unknown;
 }
 
----------------
ABataev wrote:
> Actually, I think, we need to rework the whole IsOpenMPCapturedByRef() function. I'm not sure that it is even required. It looks like some optimization hack in the frontend. I'm against any not-necessary optimizations in frontend. If scalar value is a firstprivate, it must be handled in codegen, not by handling it by copy.
'IsOpenMPCapturedByRef' goal is to change the signature of the outlined function. We don't want to have reference types in target region arguments unless they are really required. We have seen performance being greatly affected just because of that. Of course a consequence of this is to have variables that become first private.

The current implementation of OpenMP firstprivate doesn't change the the function signatures, only the codegen inside the region is affected. So, this is not a complete overlap.

With this, I am not saying that this cannot be done in codegen. The reason I am doing it here is your initial guideline that we should attempt to fix most of the things in Sema and avoid complicating codegen which is a more sensitive part.

Doing this in Codegen would require changing the implementation of `GenerateOpenMPCapturedVars` and `GenerateOpenMPCapturedStmtFunction`. We wouldn't need the tracking of the context anymore (checking the directive would be sufficient), but we would still need to see what't in the map clauses.

So, do you think we should change this?


http://reviews.llvm.org/D18110