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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
