Skip to content

[flang][OpenMP] Make parsing of trait properties more context-sensitive #122900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 29, 2025
Merged

[flang][OpenMP] Make parsing of trait properties more context-sensitive #122900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bfe74e
[flang][OpenMP] Make parsing of trait properties more context-sensitive
kparzysz Jan 14, 2025
c44051d
format/comment
kparzysz Jan 14, 2025
13740f9
add "sourced"
kparzysz Jan 14, 2025
527315f
fix parser
kparzysz Jan 15, 2025
d68017e
Merge branch 'main' into users/kparzysz/spr/m02-context-parse
kparzysz Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion flang/include/flang/Parser/dump-parse-tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ class ParseTreeDumper {
NODE(parser, OmpTraitPropertyName)
NODE(parser, OmpTraitScore)
NODE(parser, OmpTraitPropertyExtension)
NODE(OmpTraitPropertyExtension, ExtensionValue)
NODE(OmpTraitPropertyExtension, Complex)
NODE(parser, OmpTraitProperty)
NODE(parser, OmpTraitSelectorName)
NODE_ENUM(OmpTraitSelectorName, Value)
Expand Down
40 changes: 13 additions & 27 deletions flang/include/flang/Parser/parse-tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3505,37 +3505,22 @@ struct OmpTraitScore {
};

// trait-property-extension ->
// trait-property-name (trait-property-value, ...)
// trait-property-value ->
// trait-property-name |
// scalar-integer-expression |
// trait-property-extension
//
// The grammar in OpenMP 5.2+ spec is ambiguous, the above is a different
// version (but equivalent) that doesn't have ambiguities.
// The ambiguity is in
// trait-property:
// trait-property-name <- (a)
// trait-property-clause
// trait-property-expression <- (b)
// trait-property-extension <- this conflicts with (a) and (b)
// trait-property-extension:
// trait-property-name <- conflict with (a)
// identifier(trait-property-extension[, trait-property-extension[, ...]])
// constant integer expression <- conflict with (b)
// scalar-expr |
// trait-property-name (trait-property-extension, ...)
//
struct OmpTraitPropertyExtension {
CharBlock source;
TUPLE_CLASS_BOILERPLATE(OmpTraitPropertyExtension);
struct ExtensionValue {
UNION_CLASS_BOILERPLATE(OmpTraitPropertyExtension);
struct Complex { // name (prop-ext, prop-ext, ...)
CharBlock source;
UNION_CLASS_BOILERPLATE(ExtensionValue);
std::variant<OmpTraitPropertyName, ScalarExpr,
common::Indirection<OmpTraitPropertyExtension>>
u;
TUPLE_CLASS_BOILERPLATE(Complex);
std::tuple<OmpTraitPropertyName,
std::list<common::Indirection<OmpTraitPropertyExtension>>>
t;
};
using ExtensionList = std::list<ExtensionValue>;
std::tuple<OmpTraitPropertyName, ExtensionList> t;

std::variant<OmpTraitPropertyName, ScalarExpr, Complex> u;
};

// trait-property ->
Expand Down Expand Up @@ -3568,9 +3553,10 @@ struct OmpTraitProperty {
// UID | T // unique-string-id /impl-defined
// VENDOR | I // name-list (vendor-id /add-def-doc)
// EXTENSION | I // name-list (ext_name /impl-defined)
// ATOMIC_DEFAULT_MEM_ORDER I | // value of admo
// ATOMIC_DEFAULT_MEM_ORDER I | // clause-list (value of admo)
// REQUIRES | I // clause-list (from requires)
// CONDITION U // logical-expr
// <other name> I // treated as extension
//
// Trait-set-selectors:
// [D]evice, [T]arget_device, [C]onstruct, [I]mplementation, [U]ser.
Expand All @@ -3579,7 +3565,7 @@ struct OmpTraitSelectorName {
UNION_CLASS_BOILERPLATE(OmpTraitSelectorName);
ENUM_CLASS(Value, Arch, Atomic_Default_Mem_Order, Condition, Device_Num,
Extension, Isa, Kind, Requires, Simd, Uid, Vendor)
std::variant<Value, llvm::omp::Directive> u;
std::variant<Value, llvm::omp::Directive, std::string> u;
};

// trait-selector ->
Expand Down
166 changes: 122 additions & 44 deletions flang/lib/Parser/openmp-parsers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,75 +158,153 @@ static TypeDeclarationStmt makeIterSpecDecl(std::list<ObjectName> &&names) {
static std::string nameToString(Name &&name) { return name.ToString(); }

TYPE_PARSER(sourced(construct<OmpTraitPropertyName>( //
(space >> charLiteralConstantWithoutKind) ||
applyFunction(nameToString, Parser<Name>{}))))
construct<OmpTraitPropertyName>(space >> charLiteralConstantWithoutKind) ||
construct<OmpTraitPropertyName>(
applyFunction(nameToString, Parser<Name>{})))))

TYPE_PARSER(sourced(construct<OmpTraitScore>( //
"SCORE" >> parenthesized(scalarIntExpr))))
"SCORE"_id >> parenthesized(scalarIntExpr))))

TYPE_PARSER(sourced(construct<OmpTraitPropertyExtension::ExtensionValue>(
// Parse nested extension first.
construct<OmpTraitPropertyExtension::ExtensionValue>(
indirect(Parser<OmpTraitPropertyExtension>{})) ||
construct<OmpTraitPropertyExtension::ExtensionValue>(
Parser<OmpTraitPropertyName>{}) ||
construct<OmpTraitPropertyExtension::ExtensionValue>(scalarExpr))))

TYPE_PARSER(sourced(construct<OmpTraitPropertyExtension>( //
TYPE_PARSER(sourced(construct<OmpTraitPropertyExtension::Complex>(
Parser<OmpTraitPropertyName>{},
parenthesized(nonemptySeparated(
Parser<OmpTraitPropertyExtension::ExtensionValue>{}, ","_tok)))))
indirect(Parser<OmpTraitPropertyExtension>{}), ",")))))

TYPE_PARSER(sourced(construct<OmpTraitProperty>(
// Try clause first, then extension before OmpTraitPropertyName.
construct<OmpTraitProperty>(indirect(Parser<OmpClause>{})) ||
construct<OmpTraitProperty>(Parser<OmpTraitPropertyExtension>{}) ||
construct<OmpTraitProperty>(Parser<OmpTraitPropertyName>{}) ||
construct<OmpTraitProperty>(scalarExpr))))
TYPE_PARSER(sourced(construct<OmpTraitPropertyExtension>(
construct<OmpTraitPropertyExtension>(
Parser<OmpTraitPropertyExtension::Complex>{}) ||
construct<OmpTraitPropertyExtension>(Parser<OmpTraitPropertyName>{}) ||
construct<OmpTraitPropertyExtension>(scalarExpr))))

TYPE_PARSER(construct<OmpTraitSelectorName::Value>(
"ARCH" >> pure(OmpTraitSelectorName::Value::Arch) ||
"ATOMIC_DEFAULT_MEM_ORDER" >>
"ARCH"_id >> pure(OmpTraitSelectorName::Value::Arch) ||
"ATOMIC_DEFAULT_MEM_ORDER"_id >>
pure(OmpTraitSelectorName::Value::Atomic_Default_Mem_Order) ||
"CONDITION" >> pure(OmpTraitSelectorName::Value::Condition) ||
"DEVICE_NUM" >> pure(OmpTraitSelectorName::Value::Device_Num) ||
"EXTENSION" >> pure(OmpTraitSelectorName::Value::Extension) ||
"ISA" >> pure(OmpTraitSelectorName::Value::Isa) ||
"KIND" >> pure(OmpTraitSelectorName::Value::Kind) ||
"REQUIRES" >> pure(OmpTraitSelectorName::Value::Requires) ||
"SIMD" >> pure(OmpTraitSelectorName::Value::Simd) ||
"UID" >> pure(OmpTraitSelectorName::Value::Uid) ||
"VENDOR" >> pure(OmpTraitSelectorName::Value::Vendor)))
"CONDITION"_id >> pure(OmpTraitSelectorName::Value::Condition) ||
"DEVICE_NUM"_id >> pure(OmpTraitSelectorName::Value::Device_Num) ||
"EXTENSION"_id >> pure(OmpTraitSelectorName::Value::Extension) ||
"ISA"_id >> pure(OmpTraitSelectorName::Value::Isa) ||
"KIND"_id >> pure(OmpTraitSelectorName::Value::Kind) ||
"REQUIRES"_id >> pure(OmpTraitSelectorName::Value::Requires) ||
"SIMD"_id >> pure(OmpTraitSelectorName::Value::Simd) ||
"UID"_id >> pure(OmpTraitSelectorName::Value::Uid) ||
"VENDOR"_id >> pure(OmpTraitSelectorName::Value::Vendor)))
Comment on lines +180 to +191
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was using the _id combinator required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default the parser will recognize a given string when it's a prefix of another string. For example "ATOMIC" >> ... will trigger for ATOMIC_DEFAULT_MEM_ORDER, causing a syntax error which is rather non-obvious (the parser then looks at _DEFAULT_MEM_ORDER). Adding "_id" to the string ensures that it will only trigger when it's delimited.

I don't remember what exactly I ran into here, but to eliminate the risk of these kinds of issues, I added "_id" to the strings in these parsers.

Do you have concerns with this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, it is used only when a token is a prefix of another token in the same or group.


TYPE_PARSER(sourced(construct<OmpTraitSelectorName>(
// Parse predefined names first (because of SIMD).
construct<OmpTraitSelectorName>(Parser<OmpTraitSelectorName::Value>{}) ||
construct<OmpTraitSelectorName>(OmpDirectiveNameParser{}))))
construct<OmpTraitSelectorName>(OmpDirectiveNameParser{}) ||
// identifier-or-string for extensions
construct<OmpTraitSelectorName>(
applyFunction(nameToString, Parser<Name>{})) ||
construct<OmpTraitSelectorName>(space >> charLiteralConstantWithoutKind))))

// Parser for OmpTraitSelector::Properties
template <typename... PropParser>
static constexpr auto propertyListParser(PropParser... pp) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document some of these custom openmp parsers separately in the docs page?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parsers related to context selectors are still somewhat work-in-progress. I've been trying to keep them as general as possible (i.e. parsing unknown values in a reasonable manner), but once we handle the details (like arch names, etc.) the structure of these parsers may change a bit. We should document it somewhere once they stabilize.

// Parse the property list "(score(expr): item1...)" in three steps:
// 1. Parse the "("
// 2. Parse the optional "score(expr):"
// 3. Parse the "item1, ...)", together with the ")".
// The reason for including the ")" in the 3rd step is to force parsing
// the entire list in each of the alternative property parsers. Otherwise,
// the name parser could stop after "foo" in "(foo, bar(1))", without
// allowing the next parser to give the list a try.
auto listOf{[](auto parser) { //
return nonemptySeparated(parser, ",");
}};

using P = OmpTraitProperty;
return maybe("(" >> //
construct<OmpTraitSelector::Properties>(
maybe(Parser<OmpTraitScore>{} / ":"),
(attempt(listOf(sourced(construct<P>(pp))) / ")") || ...)));
}

// Parser for OmpTraitSelector
struct TraitSelectorParser {
using resultType = OmpTraitSelector;

constexpr TraitSelectorParser(Parser<OmpTraitSelectorName> p) : np(p) {}

TYPE_PARSER(construct<OmpTraitSelector::Properties>(
maybe(Parser<OmpTraitScore>{} / ":"_tok),
nonemptySeparated(Parser<OmpTraitProperty>{}, ","_tok)))
std::optional<resultType> Parse(ParseState &state) const {
auto name{attempt(np).Parse(state)};
if (!name.has_value()) {
return std::nullopt;
}

// Default fallback parser for lists that cannot be parser using the
// primary property parser.
auto extParser{Parser<OmpTraitPropertyExtension>{}};

if (auto *v{std::get_if<OmpTraitSelectorName::Value>(&name->u)}) {
// (*) The comments below show the sections of the OpenMP spec that
// describe given trait. The cases marked with a (*) are those where
// the spec doesn't assign any list-type to these traits, but for
// convenience they can be treated as if they were.
switch (*v) {
// name-list properties
case OmpTraitSelectorName::Value::Arch: // [6.0:319:18]
case OmpTraitSelectorName::Value::Extension: // [6.0:319:30]
case OmpTraitSelectorName::Value::Isa: // [6.0:319:15]
case OmpTraitSelectorName::Value::Kind: // [6.0:319:10]
case OmpTraitSelectorName::Value::Uid: // [6.0:319:23](*)
case OmpTraitSelectorName::Value::Vendor: { // [6.0:319:27]
auto pp{propertyListParser(Parser<OmpTraitPropertyName>{}, extParser)};
return OmpTraitSelector(std::move(*name), std::move(*pp.Parse(state)));
}
// clause-list
case OmpTraitSelectorName::Value::Atomic_Default_Mem_Order:
// [6.0:321:26-29](*)
case OmpTraitSelectorName::Value::Requires: // [6.0:319:33]
case OmpTraitSelectorName::Value::Simd: { // [6.0:318:31]
auto pp{propertyListParser(indirect(Parser<OmpClause>{}), extParser)};
return OmpTraitSelector(std::move(*name), std::move(*pp.Parse(state)));
}
// expr-list
case OmpTraitSelectorName::Value::Condition: // [6.0:321:33](*)
case OmpTraitSelectorName::Value::Device_Num: { // [6.0:321:23-24](*)
auto pp{propertyListParser(scalarExpr, extParser)};
return OmpTraitSelector(std::move(*name), std::move(*pp.Parse(state)));
}
} // switch
} else {
// The other alternatives are `llvm::omp::Directive`, and `std::string`.
// The former doesn't take any properties[1], the latter is a name of an
// extension[2].
// [1] [6.0:319:1-2]
// [2] [6.0:319:36-37]
auto pp{propertyListParser(extParser)};
return OmpTraitSelector(std::move(*name), std::move(*pp.Parse(state)));
}

llvm_unreachable("Unhandled trait name?");
}

private:
const Parser<OmpTraitSelectorName> np;
};

TYPE_PARSER(sourced(construct<OmpTraitSelector>( //
Parser<OmpTraitSelectorName>{}, //
maybe(parenthesized(Parser<OmpTraitSelector::Properties>{})))))
TYPE_PARSER(sourced(construct<OmpTraitSelector>(
sourced(TraitSelectorParser(Parser<OmpTraitSelectorName>{})))))

TYPE_PARSER(construct<OmpTraitSetSelectorName::Value>(
"CONSTRUCT" >> pure(OmpTraitSetSelectorName::Value::Construct) ||
"DEVICE" >> pure(OmpTraitSetSelectorName::Value::Device) ||
"IMPLEMENTATION" >> pure(OmpTraitSetSelectorName::Value::Implementation) ||
"TARGET_DEVICE" >> pure(OmpTraitSetSelectorName::Value::Target_Device) ||
"USER" >> pure(OmpTraitSetSelectorName::Value::User)))
"CONSTRUCT"_id >> pure(OmpTraitSetSelectorName::Value::Construct) ||
"DEVICE"_id >> pure(OmpTraitSetSelectorName::Value::Device) ||
"IMPLEMENTATION"_id >>
pure(OmpTraitSetSelectorName::Value::Implementation) ||
"TARGET_DEVICE"_id >> pure(OmpTraitSetSelectorName::Value::Target_Device) ||
"USER"_id >> pure(OmpTraitSetSelectorName::Value::User)))
Comment on lines +292 to +297
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same _id question here as well.


TYPE_PARSER(sourced(construct<OmpTraitSetSelectorName>(
Parser<OmpTraitSetSelectorName::Value>{})))

TYPE_PARSER(sourced(construct<OmpTraitSetSelector>( //
Parser<OmpTraitSetSelectorName>{},
"=" >> braced(nonemptySeparated(Parser<OmpTraitSelector>{}, ","_tok)))))
"=" >> braced(nonemptySeparated(Parser<OmpTraitSelector>{}, ",")))))

TYPE_PARSER(sourced(construct<OmpContextSelectorSpecification>(
nonemptySeparated(Parser<OmpTraitSetSelector>{}, ","_tok))))
nonemptySeparated(Parser<OmpTraitSetSelector>{}, ","))))

// Parser<OmpContextSelector> == Parser<traits::OmpContextSelectorSpecification>

Expand Down
5 changes: 3 additions & 2 deletions flang/lib/Parser/unparse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2075,10 +2075,11 @@ class UnparseVisitor {
Walk(x.v);
Put(")");
}
void Unparse(const OmpTraitPropertyExtension &x) {
void Unparse(const OmpTraitPropertyExtension::Complex &x) {
using PropList = std::list<common::Indirection<OmpTraitPropertyExtension>>;
Walk(std::get<OmpTraitPropertyName>(x.t));
Put("(");
Walk(std::get<OmpTraitPropertyExtension::ExtensionList>(x.t), ",");
Walk(std::get<PropList>(x.t), ",");
Put(")");
}
void Unparse(const OmpTraitSelector &x) {
Expand Down
Loading