[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

In D71830#1871838 <https://reviews.llvm.org/D71830#1871838>, @JonChesterfield wrote:

> Procedural note - adding someone as a blocking reviewer to someone else's patch isn't great. What if the new reviewer never gets around to looking at the patch?
>
> I've adjusted that to non-blocking, but feel free to leave a comment if I've missed something.


I don't know that we have any formalized procedure here, but setting a code owner as the blocking reviewer in phab makes it crystal clear that the existing LG(s) are insufficient for some reason and someone needs to do a final sign-off. It solves the problem of getting a LG from someone who may not be familiar with the code base and then making a problematic commit by accident.



================
Comment at: clang/include/clang/AST/OpenMPClause.h:6543
 
+/// Helper datastructure representing the traits in a match clause of an
+/// `declare variant` or `metadirective`. The outer level is an ordered
----------------
datastructure -> data structure


================
Comment at: clang/include/clang/AST/OpenMPClause.h:6565
+
+  /// Create a variant match infor object from this trait info object. While the
+  /// former is a flat representation the actual main difference is that the
----------------
infor -> info


================
Comment at: clang/include/clang/AST/OpenMPClause.h:6573
+
+  /// Print a humand readable representation into \p OS.
+  void print(llvm::raw_ostream &OS, const PrintingPolicy &Policy) const;
----------------
humand -> human


================
Comment at: clang/include/clang/Basic/Attr.td:185
+// An argument of type \p type with name \p name.
+class GenericPointerArgument<string name, string type> : Argument<name, 1> {
+  string Type = type;
----------------
The name seems a bit strange given that it's not a generic pointer, you specify a type with it. It's unclear whether you are supposed to specify the pointer type or the base type, which would help a bit. However, it's unclear to me why we'd want this generic mechanism in the first place. These classes are intended to help generate code to do checking automatically, and this design is somewhat at odds with that goal compared to exposing a custom argument type like we did for `VersionArgument`. Also, this type is marked optional rather than leaving that up to the caller, and the default behavior of a generic pointer argument always being default seems suspect.

I'm not saying "get rid of this", just wondering if you had considered the more verbose approach and had strong rationale as to why this was a better way.


================
Comment at: clang/include/clang/Basic/Attr.td:3351
     ExprArgument<"VariantFuncRef">,
-    VariadicExprArgument<"Scores">,
-    VariadicUnsignedArgument<"CtxSelectorSets">,
-    VariadicUnsignedArgument<"CtxSelectors">,
-    VariadicStringArgument<"ImplVendors">,
-    VariadicStringArgument<"DeviceKinds">
+    GenericPointerArgument<"TraitInfos", "OMPTraitInfo*">,
   ];
----------------
Just double-checking -- this is not a breaking change to existing code, correct?

It's not yet clear to me how a "generic pointer argument" relates to an `OMPTraitInfo` object that the end programmer will have no understanding of because it's a compiler implementation detail.

I used to understand what this attribute accepted as arguments and I no longer have the foggiest idea, which seems somewhat problematic.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1262
+def warn_omp_declare_variant_string_literal_or_identifier : Warning<
+  "expected identifier or string literal describing a context %select{set|selector|property}0, %select{set|selector|property}0 skipped">,
+  InGroup<OpenMPClauses>;
----------------
80-col limit

Also, I think the comma should be a semicolon so it reads `...describing a context set; selector skipped`


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1267
+def warn_omp_declare_variant_expected : Warning<
+  "expected '%0' after the %1, '%0' assumed">,
+  InGroup<OpenMPClauses>;
----------------
Turn the comma into a semicolon


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1270
+def warn_omp_declare_variant_ctx_not_a_property : Warning<
+  "'%0' is not a valid context property for the context selector '%1' and the context set '%2', property ignored">,
+  InGroup<OpenMPClauses>;
----------------
80-col limit; please run the patch through clang-format (I'll stop commenting on these).


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1277
+def warn_omp_declare_variant_ctx_not_a_selector : Warning<
+  "'%0' is not a valid context selector for the context set '%1', selector ignored">,
+  InGroup<OpenMPClauses>;
----------------
Turn the comma into a semicolon


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1280
+def warn_omp_declare_variant_ctx_not_a_set : Warning<
+  "'%0' is not a valid context set in a `declare variant`, set ignored">,
+  InGroup<OpenMPClauses>;
----------------
Turn the comma into a semicolon


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1283
+def warn_omp_declare_variant_ctx_mutiple_use : Warning<
+  "the context %select{set|selector|property}0 '%1' was used already in the same 'omp declare variant' directive, %select{set|selector|property}0 ignored">,
   InGroup<OpenMPClauses>;
----------------
Turn the comma into a semicolon (same comment in the other diagnostics, I'll stop commenting on these).


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1295
+def warn_omp_ctx_selector_without_properties : Warning<
+  "the context selector '%0' in context set '%1' requires a context property defined in parenthesis, selector ignored">,
+  InGroup<OpenMPClauses>;
----------------
parenthesis -> parentheses


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1710-1711
+    ASTContext &ASTCtx, llvm::omp::VariantMatchInfo &VMI) const {
+  for (const OMPTraitSet &Set : Sets) {
+    for (const OMPTraitSelector &Selector : Set.Selectors) {
+
----------------
Is there any reason to be worried about the O(N^2) here?


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1723
+        llvm::APInt CondVal =
+            Selector.ScoreOrCondition->EvaluateKnownConstInt(ASTCtx);
+        if (CondVal.isNullValue())
----------------
I didn't see anything that validated that this is known to be a constant expression. Did I miss something?

Also, is there anything checking that the expression is not value dependent?


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1724-1727
+        if (CondVal.isNullValue())
+          VMI.addTrait(llvm::omp::TraitProperty::user_condition_false);
+        else
+          VMI.addTrait(llvm::omp::TraitProperty::user_condition_true);
----------------
I think `VMI.addTrait(CondVal.isNullValue() ? ... : ...);` would be slightly more clear.


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1734
+      if (Selector.ScoreOrCondition) {
+        Score = Selector.ScoreOrCondition->EvaluateKnownConstInt(ASTCtx);
+        ScorePtr = &Score;
----------------
Same question here.


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1737
+      }
+      for (const OMPTraitProperty &Property : Selector.Properties)
+        VMI.addTrait(Set.Kind, Property.Kind, ScorePtr);
----------------
Hopefully the O(N^3) is also not a concern?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11046
+  OMPContext Ctx(CGM.getLangOpts().OpenMPIsDevice, CGM.getTriple());
+  // TODO: Keep the context in the OMPIRBuilder so we can add constructs as we
+  // build them.
----------------
Is this change planned for the patch? Or is this a FIXME suggesting a better approach we hope to take someday?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:866
+  SourceLocation NameLoc = Tok.getLocation();
+  auto StringLiteralParser = [this]() -> StringRef {
+    ExprResult Res = ParseStringLiteralExpression(true);
----------------
It is unfortunate how much this is duplicated. Perhaps make a free function to do this that gets called instead of copy pasting?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:886
+  // It follows diagnosis and helping notes.
+  // TODO: We should move the diagnosis string generation into libFrontend.
+  Diag(NameLoc, diag::warn_omp_declare_variant_ctx_not_a_property)
----------------
FIXME?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1060
+    llvm::StringMap<SourceLocation> &SeenSelectors) {
+  auto OuterPC = ParenCount;
+
----------------
Please don't use `auto` when the type is not obvious from the initialization.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1064
+  // of the selector. However, commas are ambiguous so we look for the nesting
+  // of parenthesis here as well.
+  auto FinishSelector = [OuterPC, this]() -> void {
----------------
parenthesis -> parentheses


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1379
   // Parse inner context selectors.
-  SmallVector<Sema::OMPCtxSelectorData, 4> Data;
-  if (!parseOpenMPContextSelectors(Loc, Data)) {
-    // Parse ')'.
-    (void)T.consumeClose();
-    // Need to check for extra tokens.
-    if (Tok.isNot(tok::annot_pragma_openmp_end)) {
-      Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
-          << getOpenMPDirectiveName(OMPD_declare_variant);
-    }
-  }
+  OMPTraitInfo *TI = new OMPTraitInfo();
+  parseOMPContextSelectors(Loc, *TI);
----------------
What is responsible for freeing this memory?


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:405
+  // score and condition expressiosn.
+  OMPTraitInfo *TI = new OMPTraitInfo();
+  *TI = *Attr.getTraitInfos();
----------------
What is responsible for freeing this memory?


================
Comment at: clang/lib/Serialization/ASTReader.cpp:12602
+template <> OMPTraitInfo *ASTRecordReader::readUserType() {
+  OMPTraitInfo *TI = new OMPTraitInfo();
+  TI->Sets.resize(readUInt32());
----------------
What is responsible for freeing this memory?


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:110
       .Case("ParamIdx", "ParamIdx::deserialize(Record.readInt())")
+      .EndsWith("*", "Record.readUserType<" +
+                         std::string(type.data(), 0, type.size() - 1) + ">()")
----------------
The idea here being that if someone adds a `GenericPointerArg`, they'll have to add a specialization of `readUserType<>` for that named pointer type or else get template instantiation errors because there's no definition for the primary template?


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:367
       } else {
-        llvm_unreachable("Unknown SimpleArgument type!");
+        OS << "    if (SA->get" << getUpperName() << "())\n  ";
+        OS << "      OS << \" \" << *SA->get" << getUpperName() << "();\n";
----------------
Can we leave the unreachable in there and add actual checking for this case? The unreachable is intended to remind people "do this work when you're mucking about in here" and silently doing something that may or may not be wrong makes this harder to spot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71830