[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 12 13:38:01 PST 2020
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
More information about the llvm-commits
mailing list