arpith-jacob added inline comments.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:84-115
+/// Lexical scope for OpenMP parallel construct, that handles correct codegen
+/// for captured expressions.
+class OMPParallelScope final : public CodeGenFunction::LexicalScope {
+  void emitPreInitStmt(CodeGenFunction &CGF, const OMPExecutableDirective &S) {
+    OpenMPDirectiveKind Kind = S.getDirectiveKind();
+    if (!isOpenMPTargetExecutionDirective(Kind) &&
+        isOpenMPParallelDirective(Kind)) {
----------------
arpith-jacob wrote:
> ABataev wrote:
> > Could you join it with `OMPLexicalScope` somehow? I don't like the idea of 
> > adding a different kind of scope for parallel and other directives.
> > Or follow the solution for `OMPLoopScope`, rather than for 
> > `OMPLexicalScope`.
> Hi Alexey, thanks for looking at this patch.
> 
> We need a different kind of scope for 'parallel', however, we don't need a 
> different scope for any other directive so the code additions should be 
> limited.  Here is my idea for merging it with OMPLexicalScope to reduce code 
> duplication.
> 
> I can make OMPParallelScope derive from OMPLexicalScope.  I'll make 
> emitPreInitStmt a virtual class that can be overridden.  
> 
> ```
> class OMPParallelScope final : public OMPLexicalScope {
>   void emitPreInitStmt(CodeGenFunction &CGF, const OMPExecutableDirective &S) 
> override {
>     OpenMPDirectiveKind Kind = S.getDirectiveKind();
>     if (!isOpenMPTargetExecutionDirective(Kind) &&
>         isOpenMPParallelDirective(Kind))
>         OMPParallelScope::emitPreInitStmt(CGF, S);
>   }
> ...
> }
> ```
> 
> Does that look ok?
Sorry, the code should read:

class OMPParallelScope final : public OMPLexicalScope {
  void emitPreInitStmt(CodeGenFunction &CGF, const OMPExecutableDirective &S) 
override {
    OpenMPDirectiveKind Kind = S.getDirectiveKind();
    if (!isOpenMPTargetExecutionDirective(Kind) &&
        isOpenMPParallelDirective(Kind))
        //**OMPLexicalScope**//::emitPreInitStmt(CGF, S);
  }
...
}


https://reviews.llvm.org/D28781



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to