[PATCH] D28781: [OpenMP] Support for the if-clause on the combined directive 'target parallel'.

[Arpith Jacob via Phabricator via cfe-commits]

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)) {
----------------
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?


https://reviews.llvm.org/D28781