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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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;
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > 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.
> > The name seems a bit strange given that it's not a generic pointer, you specify a type with it.
> 
> The "template" is generic as it accepts any pointer type. All the code that work with `GenericPointerArgument` don't know what pointer it is, just that it is called "Type". Don't you think that is generic?
> 
> 
> > 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.
> 
> That's an oversight. I mark it non-optional. I don't get the "always being default" part.
> 
> > However, it's unclear to me why we'd want this generic mechanism in the first place. 
> > [...]
> > 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.
> 
> The "problem" is that we want to keep "complex" information around. The verbose approach is what we are doing right now for a subset of the information. For this subset we already track various variadic expr, variadic unsigned, and variadic string arguments and stitch them together later with complex and careful iteration over all containers at the same time. It's now 5 variadic XXX arguments and it would be >= 7 to support what this approach does.
> 
> This approach subsumes this as we can retain the original structure/nesting in the custom `OMPTraitInfo` object that is part of the `OMPDeclareVariant` instead. [FWIW, in the review for the verbose code we have now, in case you haven't seen that, I asked for a less verbose method to store the information because the iteration over the stuff, once flattend, is so brittle.]
> 
> That said, we can make a specialized argument here instead, e.g., OMPTraitInforArgument, which contains a OMPTraitInfo pointer. I figured this might not be the last time someone wants to keep a complex structure inside an attribute argument and creating new arguments all the time seems a lot of waste (due to all the boilerplate). If you think we should go that route, I will happily try it out.
> 
> (As noted below somewhere, this method avoids a lot of boilerplate by requiring a specialization of one AST reader/writer method.)
> That's an oversight. I mark it non-optional. I don't get the "always being default" part.

Making it non-optional solves my concern. I was just worried that the default was surprising.

> That said, we can make a specialized argument here instead, e.g., OMPTraitInforArgument, which contains a OMPTraitInfo pointer. I figured this might not be the last time someone wants to keep a complex structure inside an attribute argument and creating new arguments all the time seems a lot of waste (due to all the boilerplate). If you think we should go that route, I will happily try it out.

As we spoke about on IRC, I would appreciate going with this approach (and I think it can even work nicely with the template specialization work done for the AST reader and writer, most likely). However, if that turns out to be a lot of effort for you, I can probably be okay with the current approach. I just dislike the fact that it complicates understanding what arguments the attribute actually takes (I no longer understand by looking at Attr.td or in the worst case, the tablegen emitter). Having the concrete type in tablegen/emitter is more expressive and allows us to generate more stuff in the future.


================
Comment at: clang/include/clang/Basic/Attr.td:3351
     ExprArgument<"VariantFuncRef">,
-    VariadicExprArgument<"Scores">,
-    VariadicUnsignedArgument<"CtxSelectorSets">,
-    VariadicUnsignedArgument<"CtxSelectors">,
-    VariadicStringArgument<"ImplVendors">,
-    VariadicStringArgument<"DeviceKinds">
+    GenericPointerArgument<"TraitInfos", "OMPTraitInfo*">,
   ];
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > 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.
> > Just double-checking -- this is not a breaking change to existing code, correct?
> 
> This actually fixes various errors in the existing code and adds a lot of missing functionality, no regressions are known to me.
> 
> > 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.
> 
> This is not (supposed to be) user facing. This is generated from the `...` in `omp declare variant match(...)`.
> 
> > I used to understand what this attribute accepted as arguments and I no longer have the foggiest idea, which seems somewhat problematic.
> 
> I don't understand? The attribute accepted a custom (=internal) encoding of an OpenMP context selector as a sequence of expressions, unsigneds, and strings. I do honestly not understand, without significant effort, how the nesting is rebuild from 5 such lists, e.g., how the iterators in the `printPrettyPragma` on the left (line 3359) work together. I argue `OMPTraitInfo::print` is much simpler because the nesting is retained.
> 
>> I used to understand what this attribute accepted as arguments and I no longer have the foggiest idea, which seems somewhat problematic.
> I don't understand? 

My confusion comes from me thinking about arguments in the .td file as being descriptive of how a user will use the attribute (same for subjects and many other properties we set up on the attributes). So things like arguments are intended to be descriptive and self-documenting. In this case, it's no longer self-documenting or descriptive, or even user facing.


================
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>;
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > 80-col limit; please run the patch through clang-format (I'll stop commenting on these).
> maybe we should tell git-clang-format to run on .td files (in the llvm subfolder)
Not a bad idea!


================
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) {
+
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > Is there any reason to be worried about the O(N^2) here?
> I don't think so. (1) There are only four sets, and a limited number of selectors. And, (2) all this does is go over user typed things which is not quadratic in the input size anyway. I means `Sets` and `Set.Selectors` are filled with things the user explicitly typed in a `match` clause.
Fantastic! That's what I was hoping to hear.


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1723
+        llvm::APInt CondVal =
+            Selector.ScoreOrCondition->EvaluateKnownConstInt(ASTCtx);
+        if (CondVal.isNullValue())
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > 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?
> > I didn't see anything that validated that this is known to be a constant expression. Did I miss something? 
> 
> I'll look into that and make sure we check in the parser.
> 
> > Also, is there anything checking that the expression is not value dependent?
> 
> That should be fine. If it is attached to a template it is instantiated with the template. Or am I missing something?
> That should be fine. If it is attached to a template it is instantiated with the template. Or am I missing something?

`EvaluateKnownConstInt()` asserts that the expression is not value dependent, so I just wanted to be certain that assertion wouldn't trigger. Sounds like it won't because this should only be called on an instantiation?


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1737
+      }
+      for (const OMPTraitProperty &Property : Selector.Properties)
+        VMI.addTrait(Set.Kind, Property.Kind, ScorePtr);
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > Hopefully the O(N^3) is also not a concern?
> Same answer as for the `N^2`, this is only iterating over a user defined structure, not all possible values in an `N^3` square.
Fantastic, thank you!


================
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.
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > Is this change planned for the patch? Or is this a FIXME suggesting a better approach we hope to take someday?
> It is something we need to do *soon* not someday, though it will only make sense as the OMPIRBuilder is gaining the ability to fully generate constructs.
Ah, cool -- slight preference for using "FIXME" to describe those sort of situations. TODO sounds like someone forgot to do it rather than someone recognizing it needs to be fixed.


================
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) + ">()")
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > 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?
> Correct.
I like it. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71830