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

[Alexey Bataev via cfe-commits]

ABataev added inline comments.

================
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;
 }
 
----------------
sfantao wrote:
> 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?
I think we should not dot it at all for now. Performance is an important thing, of course, but at first we must have just working solution and only after that we will think about performance.
Early optimization leads to many troubles. The code becomes very hard to understand. It just increases the time of the development.
I suggest to remove all optimizations for now and emit the code as is, before we have a working solution.
Also, this function may lead to incorrect codegen. You don't check that CapturedRegion is actually an OpenMP region. If we have non-OpenMP cpatured region between OpenMP regions it will lead to incorrect code.


http://reviews.llvm.org/D18110