[Openmp-commits] [PATCH] D76211: OpenMP Metadirective with user defined condition

Johannes Doerfert via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Tue Aug 11 20:54:43 PDT 2020

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
This is not rebased, directives are now declared in the tablegen file:

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list