[PATCH] D91944: OpenMP 5.0 metadirective

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1952
-      STMT_MS_DEPENDENT_EXISTS,   // MSDependentExistsStmt
-      EXPR_LAMBDA,                // LambdaExpr
       STMT_COROUTINE_BODY,
----------------
alokmishra.besu wrote:
> jdoerfert wrote:
> > Unrelated.
> Only STMT_OMP_META_DIRECTIVE was added. Rest was formatted by git clang-format
> Rest was formatted by git clang-format

Please don't format unrelated code. 


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2264
+          Actions.StartOpenMPDSABlock(DirKind, DirName, Actions.getCurScope(),
+                                      Loc);
+          int paren = 0;
----------------
alokmishra.besu wrote:
> jdoerfert wrote:
> > Should we not go back to the original code handling "directives" instead? This looks like it is copied here.
> Unfortunately we cannot go to the original code handling since the original code handling assumes that the directive always ends with annot_pragma_openmp_end, while here it will always end with ')'.
> In specification 5.0, since we are choosing only 1 directive, the body of the while block remains the same as the original code. Only the condition of the while block changes. In specification 5.1, we will need to generate code for dynamic handling and even the body will differ as we might need to generate AST node for multiple directives. It is best if we handle this code here for easier handling of 5.1 code, than in the original code space.
> I will add a TODO comment here.
> Unfortunately we cannot go to the original code handling since the original code handling assumes that the directive always ends with annot_pragma_openmp_end, while here it will always end with ')'.

Let's add a flag to the original handling to make this possible then. Copying it is going to create more long term problems.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:337
+    const SmallVectorImpl<VariantMatchInfo> &VMIs, const OMPContext &Ctx,
+    SmallVectorImpl<unsigned> *OrderedMatch) {
+
----------------
alokmishra.besu wrote:
> jdoerfert wrote:
> > This looks like a clone of `getBestVariantMatchForContext` with an extra unused argument.
> I intended to keep it similar for 5.0 to be updated in 5.1 code. But anyways it seems to be giving wrong result. Will go through this again.
Use the `getBestVariantMatchForContext` interface now and for the 5.1 semantics we introduce the `OrderedMatch`. That can also be used in the call site declare variant handling which currently calls `getBestVariantMatchForContext` multiple times.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91944