[PATCH] D91944: OpenMP 5.0 metadirective

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

This looks close to an OpenMP 5.0 implementation. I left comments inlined.

We need tests that show how non-selected alternatives *do not* impact the program. As an example, a template instantiation inside of a non-selected alternative is not actually performed.

We also need test with ill-formed metadirectives.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10493
+  "misplaced default clause! Only one default clause is allowed in "
+  "metadirective in the end">;
 } // end of OpenMP category
----------------
no `!`. The default clause doesn't need to be in the end either.


================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1952
-      STMT_MS_DEPENDENT_EXISTS,   // MSDependentExistsStmt
-      EXPR_LAMBDA,                // LambdaExpr
       STMT_COROUTINE_BODY,
----------------
Unrelated.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:10211
+    return;
+  }
+
----------------
Can you explain this, this seems odd to me.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2186
+        // parse and get condition expression to pass to the When clause
+        parseOMPContextSelectors(Loc, TI);
+
----------------
Usually you would check the return value in case we later actually propagate errors while parsing the context selector.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2193
+          Diag(Tok, diag::warn_pragma_expected_colon) << "when clause";
+          return Directive;
+        }
----------------
If we give up it should be an error, I think. If we issue a warning we just pretend the colon was there afterwards.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2204
+        ConsumeAnyToken();
+      }
+      // Parse ')'
----------------
We have balanced trackers for this that also deal with the fact that there might be a `)` missing. This code will probably crash.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2216
+    TPA.Revert();
+    TargetOMPContext OMPCtx(ASTContext, nullptr, nullptr);
+    int BestIdx = getBestWhenMatchForContext(VMIs, OMPCtx);
----------------
Add a TODO that `nullptr` should be replaced as per the use in `Sema::ActOnOpenMPCall`.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2237
+        continue;
+      }
+
----------------
Use a BalancedDelimiterTracker.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2254
+        // Parse ':'
+        ConsumeAnyToken();
+      }
----------------
If you warn and continue above you need to check for `:` here again.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2260
+        DirKind = parseOpenMPDirectiveKind(*this);
+        ConsumeToken();
+        if (DirKind != OMPD_unknown) {
----------------
What is this token? We have skipped this part before so we need to validate it is what we expect it to be.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2264
+          Actions.StartOpenMPDSABlock(DirKind, DirName, Actions.getCurScope(),
+                                      Loc);
+          int paren = 0;
----------------
Should we not go back to the original code handling "directives" instead? This looks like it is copied here.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2317
+          ConsumeAnnotationToken();
+        }
+      } else {
----------------
Same as below, change the order. Also, the "skipping" part is always the same, put it in a helper function or lambda.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2324
+        ConsumeAnnotationToken();
+      }
+      break;
----------------
Move the smaller case first and use an early exit. That will reduce the indention of the larger case by 1.


================
Comment at: clang/test/OpenMP/metadirective_construct.cpp:12
+    when(construct = {"target"} \
+         : distribute parallel for) default()
+  for (int i = 0; i < N; i++)
----------------
Since when does the `construct` trait work? I'm confused.


================
Comment at: clang/test/OpenMP/metadirective_empty.cpp:1
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
----------------
no -fopenmp-targets please.


================
Comment at: clang/test/OpenMP/metadirective_implementation.cpp:1
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
----------------
Can we run this for all configurations of the metadirective so we can actually see it will pick the right one, not the first?


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPContext.h:197
+// int getBestWhenMatchForContext(const SmallVectorImpl<VariantMatchInfo> &VMIs,
+//                               const OMPContext &Ctx);
 /// Return the index (into \p VMIs) of the variant with the highest score
----------------
Leftover.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:337
+    const SmallVectorImpl<VariantMatchInfo> &VMIs, const OMPContext &Ctx,
+    SmallVectorImpl<unsigned> *OrderedMatch) {
+
----------------
This looks like a clone of `getBestVariantMatchForContext` with an extra unused argument.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:400
+  return BestVMIIdx;
+}*/
+
----------------
Leftover.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91944