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