jdoerfert marked 14 inline comments as done.
jdoerfert 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;
----------------
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.)
================
Comment at: clang/include/clang/Basic/Attr.td:3351
ExprArgument<"VariantFuncRef">,
- VariadicExprArgument<"Scores">,
- VariadicUnsignedArgument<"CtxSelectorSets">,
- VariadicUnsignedArgument<"CtxSelectors">,
- VariadicStringArgument<"ImplVendors">,
- VariadicStringArgument<"DeviceKinds">
+ GenericPointerArgument<"TraitInfos", "OMPTraitInfo*">,
];
----------------
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.
================
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>;
----------------
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)
================
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) {
+
----------------
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.
================
Comment at: clang/lib/AST/OpenMPClause.cpp:1723
+ llvm::APInt CondVal =
+ Selector.ScoreOrCondition->EvaluateKnownConstInt(ASTCtx);
+ if (CondVal.isNullValue())
----------------
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?
================
Comment at: clang/lib/AST/OpenMPClause.cpp:1734
+ if (Selector.ScoreOrCondition) {
+ Score = Selector.ScoreOrCondition->EvaluateKnownConstInt(ASTCtx);
+ ScorePtr = &Score;
----------------
aaron.ballman wrote:
> Same question here.
> Same question here.
I'll look into that and make sure we check in the parser.
================
Comment at: clang/lib/AST/OpenMPClause.cpp:1737
+ }
+ for (const OMPTraitProperty &Property : Selector.Properties)
+ VMI.addTrait(Set.Kind, Property.Kind, ScorePtr);
----------------
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.
================
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.
----------------
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.
================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:866
+ SourceLocation NameLoc = Tok.getLocation();
+ auto StringLiteralParser = [this]() -> StringRef {
+ ExprResult Res = ParseStringLiteralExpression(true);
----------------
aaron.ballman wrote:
> It is unfortunate how much this is duplicated. Perhaps make a free function
> to do this that gets called instead of copy pasting?
Will do.
================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1060
+ llvm::StringMap<SourceLocation> &SeenSelectors) {
+ auto OuterPC = ParenCount;
+
----------------
aaron.ballman wrote:
> Please don't use `auto` when the type is not obvious from the initialization.
I want to avoid casting `ParenCount` and keep it in sync. I can copy whatever
`ParenCount` is though.
================
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);
----------------
aaron.ballman wrote:
> What is responsible for freeing this memory?
Will check and fix if needed.
================
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) + ">()")
----------------
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.
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
