[PATCH] D76211: OpenMP Metadirective with user defined condition

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.
Herald added subscribers: llvm-commits, sstefan1.
Herald added a project: LLVM.

Are you planning to add support for the actual OpenMP selectors as well? 
All the other traits, scoring, etc. I left a comment below that points you to the right place as we have almost all of that already ready to be reused.
On this note, I would recommend you take a look at the OpenMP 5.1 draft, it contains proper wording for a dynamic context extension, and we will need support in the declare variant as well. That means we need to allow dynamic expressions in `OMPTraitInfo` and similar places eventually anyway.

We need more tests:

- verify all parser errors
- verify semantic errors wrt the directives
- verify the condition is captured properly, e.g., if it is in a parallel region
- verify complex conditions work, e.g., globals, calls, constexpr expressions, template arguments, ...
- verify an "empty" metadirective works
- verify we exclude directives & conditions that should not be emitted

> Fixed an issue where correct code was not generated if directive variant was not provided in when/default clause

We need a test for that too.



================
Comment at: clang/include/clang-c/Index.h:2581
 
-  CXCursor_LastStmt = CXCursor_OMPDepobjDirective,
+  CXCursor_OMPMetaDirective = 287,
+
----------------
Doc string missing, see above.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:866
+
+  Expr *CondExpr;
+  OpenMPDirectiveKind DKind;
----------------
This looks like it is tailored towards a single user condition. You should use a `OMPTraitInfo` object here, see how it is done for declare variant (which has the same kind of context selector): https://github.com/llvm/llvm-project/blob/master/clang/lib/Parse/ParseOpenMP.cpp#L1388


================
Comment at: clang/include/clang/AST/OpenMPClause.h:888
+      : OMPClause(OMPC_when, StartLoc, EndLoc), CondExpr(expr), DKind(dKind),
+        Clauses(clauses), LParenLoc(LParenLoc) {}
+
----------------
The comment above refers to `A` and looks pretty much like it is copy and pasted from elsewhere.


================
Comment at: clang/include/clang/AST/OpenMPClause.h:903
+  /// Returns the directive variant kind
+  OpenMPDirectiveKind getDKind() { return DKind; }
+
----------------
Why does the clause have a directive kind?


================
Comment at: clang/include/clang/AST/OpenMPClause.h:868
+  OpenMPDirectiveKind DKind;
+  SmallVector<OMPClause *, 5> Clauses;
+
----------------
ABataev wrote:
> Bad idea. If you need to store other clauses in this clause, use tail allocation.
This doesn't seem to be addressed but I'm actually unsure what the clauses of a when clause are?



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10133
+  "misplaced default clause! Only one default clause is allowed in metadirective in the end"
+  >;
 } // end of OpenMP category
----------------
Unsure about the bang above. We should probably have other errors of this kind with wording you can use. Also, the order of clauses is not important (for almost everything).


================
Comment at: clang/test/OpenMP/metadirective_codegen.cpp:39
+  return 0;
+}
----------------
We need to check for the code of the outlined functions as well, from this it is not clear that there is a `omp for` in the second case. The formatting of these check lines is also problematic. Maybe start by using the `llvm/utils/update_cc_test_checks.py` script.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:95
 __OMP_DIRECTIVE(depobj)
+__OMP_DIRECTIVE(metadirective)
 
----------------
This is not rebased, directives are now declared in the tablegen file:
`llvm/include/llvm/Frontend/OpenMP/OMP.td`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76211/new/

https://reviews.llvm.org/D76211