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