Skip to content

Commit c859323

Browse files
authored
[flang][OpenMP] Make parsing of trait properties more context-sensitive (#122900)
A trait poperty can be one of serveral alternatives (name, expression, etc.), and each property in a list was parsed as if it could be any of these alternatives independently from other properties. This made the parsing vulnerable to certain ambiguities in the trait grammar (provided in the OpenMP spec). At the same time the OpenMP spec gives the expected types of properties for almost every trait: all properties listed for a given trait are usually of the same type, e.g. names, clauses, etc. Incorporate these restrictions into the parser, and additionally use property extensions as the fallback if the parsing of the expected property type failed. This is intended to allow the parser to succeed, and instead let the semantic-checking code emit a more user-friendly message.
1 parent aa29521 commit c859323

File tree

4 files changed

+139
-74
lines changed

4 files changed

+139
-74
lines changed

flang/include/flang/Parser/dump-parse-tree.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,7 @@ class ParseTreeDumper {
480480
NODE(parser, OmpTraitPropertyName)
481481
NODE(parser, OmpTraitScore)
482482
NODE(parser, OmpTraitPropertyExtension)
483-
NODE(OmpTraitPropertyExtension, ExtensionValue)
483+
NODE(OmpTraitPropertyExtension, Complex)
484484
NODE(parser, OmpTraitProperty)
485485
NODE(parser, OmpTraitSelectorName)
486486
NODE_ENUM(OmpTraitSelectorName, Value)

flang/include/flang/Parser/parse-tree.h

Lines changed: 13 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3508,37 +3508,22 @@ struct OmpTraitScore {
35083508
};
35093509

35103510
// trait-property-extension ->
3511-
// trait-property-name (trait-property-value, ...)
3512-
// trait-property-value ->
35133511
// trait-property-name |
3514-
// scalar-integer-expression |
3515-
// trait-property-extension
3516-
//
3517-
// The grammar in OpenMP 5.2+ spec is ambiguous, the above is a different
3518-
// version (but equivalent) that doesn't have ambiguities.
3519-
// The ambiguity is in
3520-
// trait-property:
3521-
// trait-property-name <- (a)
3522-
// trait-property-clause
3523-
// trait-property-expression <- (b)
3524-
// trait-property-extension <- this conflicts with (a) and (b)
3525-
// trait-property-extension:
3526-
// trait-property-name <- conflict with (a)
3527-
// identifier(trait-property-extension[, trait-property-extension[, ...]])
3528-
// constant integer expression <- conflict with (b)
3512+
// scalar-expr |
3513+
// trait-property-name (trait-property-extension, ...)
35293514
//
35303515
struct OmpTraitPropertyExtension {
35313516
CharBlock source;
3532-
TUPLE_CLASS_BOILERPLATE(OmpTraitPropertyExtension);
3533-
struct ExtensionValue {
3517+
UNION_CLASS_BOILERPLATE(OmpTraitPropertyExtension);
3518+
struct Complex { // name (prop-ext, prop-ext, ...)
35343519
CharBlock source;
3535-
UNION_CLASS_BOILERPLATE(ExtensionValue);
3536-
std::variant<OmpTraitPropertyName, ScalarExpr,
3537-
common::Indirection<OmpTraitPropertyExtension>>
3538-
u;
3520+
TUPLE_CLASS_BOILERPLATE(Complex);
3521+
std::tuple<OmpTraitPropertyName,
3522+
std::list<common::Indirection<OmpTraitPropertyExtension>>>
3523+
t;
35393524
};
3540-
using ExtensionList = std::list<ExtensionValue>;
3541-
std::tuple<OmpTraitPropertyName, ExtensionList> t;
3525+
3526+
std::variant<OmpTraitPropertyName, ScalarExpr, Complex> u;
35423527
};
35433528

35443529
// trait-property ->
@@ -3571,9 +3556,10 @@ struct OmpTraitProperty {
35713556
// UID | T // unique-string-id /impl-defined
35723557
// VENDOR | I // name-list (vendor-id /add-def-doc)
35733558
// EXTENSION | I // name-list (ext_name /impl-defined)
3574-
// ATOMIC_DEFAULT_MEM_ORDER I | // value of admo
3559+
// ATOMIC_DEFAULT_MEM_ORDER I | // clause-list (value of admo)
35753560
// REQUIRES | I // clause-list (from requires)
35763561
// CONDITION U // logical-expr
3562+
// <other name> I // treated as extension
35773563
//
35783564
// Trait-set-selectors:
35793565
// [D]evice, [T]arget_device, [C]onstruct, [I]mplementation, [U]ser.
@@ -3582,7 +3568,7 @@ struct OmpTraitSelectorName {
35823568
UNION_CLASS_BOILERPLATE(OmpTraitSelectorName);
35833569
ENUM_CLASS(Value, Arch, Atomic_Default_Mem_Order, Condition, Device_Num,
35843570
Extension, Isa, Kind, Requires, Simd, Uid, Vendor)
3585-
std::variant<Value, llvm::omp::Directive> u;
3571+
std::variant<Value, llvm::omp::Directive, std::string> u;
35863572
};
35873573

35883574
// trait-selector ->

flang/lib/Parser/openmp-parsers.cpp

Lines changed: 122 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -158,75 +158,153 @@ static TypeDeclarationStmt makeIterSpecDecl(std::list<ObjectName> &&names) {
158158
static std::string nameToString(Name &&name) { return name.ToString(); }
159159

160160
TYPE_PARSER(sourced(construct<OmpTraitPropertyName>( //
161-
(space >> charLiteralConstantWithoutKind) ||
162-
applyFunction(nameToString, Parser<Name>{}))))
161+
construct<OmpTraitPropertyName>(space >> charLiteralConstantWithoutKind) ||
162+
construct<OmpTraitPropertyName>(
163+
applyFunction(nameToString, Parser<Name>{})))))
163164

164165
TYPE_PARSER(sourced(construct<OmpTraitScore>( //
165-
"SCORE" >> parenthesized(scalarIntExpr))))
166+
"SCORE"_id >> parenthesized(scalarIntExpr))))
166167

167-
TYPE_PARSER(sourced(construct<OmpTraitPropertyExtension::ExtensionValue>(
168-
// Parse nested extension first.
169-
construct<OmpTraitPropertyExtension::ExtensionValue>(
170-
indirect(Parser<OmpTraitPropertyExtension>{})) ||
171-
construct<OmpTraitPropertyExtension::ExtensionValue>(
172-
Parser<OmpTraitPropertyName>{}) ||
173-
construct<OmpTraitPropertyExtension::ExtensionValue>(scalarExpr))))
174-
175-
TYPE_PARSER(sourced(construct<OmpTraitPropertyExtension>( //
168+
TYPE_PARSER(sourced(construct<OmpTraitPropertyExtension::Complex>(
176169
Parser<OmpTraitPropertyName>{},
177170
parenthesized(nonemptySeparated(
178-
Parser<OmpTraitPropertyExtension::ExtensionValue>{}, ","_tok)))))
171+
indirect(Parser<OmpTraitPropertyExtension>{}), ",")))))
179172

180-
TYPE_PARSER(sourced(construct<OmpTraitProperty>(
181-
// Try clause first, then extension before OmpTraitPropertyName.
182-
construct<OmpTraitProperty>(indirect(Parser<OmpClause>{})) ||
183-
construct<OmpTraitProperty>(Parser<OmpTraitPropertyExtension>{}) ||
184-
construct<OmpTraitProperty>(Parser<OmpTraitPropertyName>{}) ||
185-
construct<OmpTraitProperty>(scalarExpr))))
173+
TYPE_PARSER(sourced(construct<OmpTraitPropertyExtension>(
174+
construct<OmpTraitPropertyExtension>(
175+
Parser<OmpTraitPropertyExtension::Complex>{}) ||
176+
construct<OmpTraitPropertyExtension>(Parser<OmpTraitPropertyName>{}) ||
177+
construct<OmpTraitPropertyExtension>(scalarExpr))))
186178

187179
TYPE_PARSER(construct<OmpTraitSelectorName::Value>(
188-
"ARCH" >> pure(OmpTraitSelectorName::Value::Arch) ||
189-
"ATOMIC_DEFAULT_MEM_ORDER" >>
180+
"ARCH"_id >> pure(OmpTraitSelectorName::Value::Arch) ||
181+
"ATOMIC_DEFAULT_MEM_ORDER"_id >>
190182
pure(OmpTraitSelectorName::Value::Atomic_Default_Mem_Order) ||
191-
"CONDITION" >> pure(OmpTraitSelectorName::Value::Condition) ||
192-
"DEVICE_NUM" >> pure(OmpTraitSelectorName::Value::Device_Num) ||
193-
"EXTENSION" >> pure(OmpTraitSelectorName::Value::Extension) ||
194-
"ISA" >> pure(OmpTraitSelectorName::Value::Isa) ||
195-
"KIND" >> pure(OmpTraitSelectorName::Value::Kind) ||
196-
"REQUIRES" >> pure(OmpTraitSelectorName::Value::Requires) ||
197-
"SIMD" >> pure(OmpTraitSelectorName::Value::Simd) ||
198-
"UID" >> pure(OmpTraitSelectorName::Value::Uid) ||
199-
"VENDOR" >> pure(OmpTraitSelectorName::Value::Vendor)))
183+
"CONDITION"_id >> pure(OmpTraitSelectorName::Value::Condition) ||
184+
"DEVICE_NUM"_id >> pure(OmpTraitSelectorName::Value::Device_Num) ||
185+
"EXTENSION"_id >> pure(OmpTraitSelectorName::Value::Extension) ||
186+
"ISA"_id >> pure(OmpTraitSelectorName::Value::Isa) ||
187+
"KIND"_id >> pure(OmpTraitSelectorName::Value::Kind) ||
188+
"REQUIRES"_id >> pure(OmpTraitSelectorName::Value::Requires) ||
189+
"SIMD"_id >> pure(OmpTraitSelectorName::Value::Simd) ||
190+
"UID"_id >> pure(OmpTraitSelectorName::Value::Uid) ||
191+
"VENDOR"_id >> pure(OmpTraitSelectorName::Value::Vendor)))
200192

201193
TYPE_PARSER(sourced(construct<OmpTraitSelectorName>(
202194
// Parse predefined names first (because of SIMD).
203195
construct<OmpTraitSelectorName>(Parser<OmpTraitSelectorName::Value>{}) ||
204-
construct<OmpTraitSelectorName>(OmpDirectiveNameParser{}))))
196+
construct<OmpTraitSelectorName>(OmpDirectiveNameParser{}) ||
197+
// identifier-or-string for extensions
198+
construct<OmpTraitSelectorName>(
199+
applyFunction(nameToString, Parser<Name>{})) ||
200+
construct<OmpTraitSelectorName>(space >> charLiteralConstantWithoutKind))))
201+
202+
// Parser for OmpTraitSelector::Properties
203+
template <typename... PropParser>
204+
static constexpr auto propertyListParser(PropParser... pp) {
205+
// Parse the property list "(score(expr): item1...)" in three steps:
206+
// 1. Parse the "("
207+
// 2. Parse the optional "score(expr):"
208+
// 3. Parse the "item1, ...)", together with the ")".
209+
// The reason for including the ")" in the 3rd step is to force parsing
210+
// the entire list in each of the alternative property parsers. Otherwise,
211+
// the name parser could stop after "foo" in "(foo, bar(1))", without
212+
// allowing the next parser to give the list a try.
213+
auto listOf{[](auto parser) { //
214+
return nonemptySeparated(parser, ",");
215+
}};
216+
217+
using P = OmpTraitProperty;
218+
return maybe("(" >> //
219+
construct<OmpTraitSelector::Properties>(
220+
maybe(Parser<OmpTraitScore>{} / ":"),
221+
(attempt(listOf(sourced(construct<P>(pp))) / ")") || ...)));
222+
}
223+
224+
// Parser for OmpTraitSelector
225+
struct TraitSelectorParser {
226+
using resultType = OmpTraitSelector;
227+
228+
constexpr TraitSelectorParser(Parser<OmpTraitSelectorName> p) : np(p) {}
205229

206-
TYPE_PARSER(construct<OmpTraitSelector::Properties>(
207-
maybe(Parser<OmpTraitScore>{} / ":"_tok),
208-
nonemptySeparated(Parser<OmpTraitProperty>{}, ","_tok)))
230+
std::optional<resultType> Parse(ParseState &state) const {
231+
auto name{attempt(np).Parse(state)};
232+
if (!name.has_value()) {
233+
return std::nullopt;
234+
}
235+
236+
// Default fallback parser for lists that cannot be parser using the
237+
// primary property parser.
238+
auto extParser{Parser<OmpTraitPropertyExtension>{}};
239+
240+
if (auto *v{std::get_if<OmpTraitSelectorName::Value>(&name->u)}) {
241+
// (*) The comments below show the sections of the OpenMP spec that
242+
// describe given trait. The cases marked with a (*) are those where
243+
// the spec doesn't assign any list-type to these traits, but for
244+
// convenience they can be treated as if they were.
245+
switch (*v) {
246+
// name-list properties
247+
case OmpTraitSelectorName::Value::Arch: // [6.0:319:18]
248+
case OmpTraitSelectorName::Value::Extension: // [6.0:319:30]
249+
case OmpTraitSelectorName::Value::Isa: // [6.0:319:15]
250+
case OmpTraitSelectorName::Value::Kind: // [6.0:319:10]
251+
case OmpTraitSelectorName::Value::Uid: // [6.0:319:23](*)
252+
case OmpTraitSelectorName::Value::Vendor: { // [6.0:319:27]
253+
auto pp{propertyListParser(Parser<OmpTraitPropertyName>{}, extParser)};
254+
return OmpTraitSelector(std::move(*name), std::move(*pp.Parse(state)));
255+
}
256+
// clause-list
257+
case OmpTraitSelectorName::Value::Atomic_Default_Mem_Order:
258+
// [6.0:321:26-29](*)
259+
case OmpTraitSelectorName::Value::Requires: // [6.0:319:33]
260+
case OmpTraitSelectorName::Value::Simd: { // [6.0:318:31]
261+
auto pp{propertyListParser(indirect(Parser<OmpClause>{}), extParser)};
262+
return OmpTraitSelector(std::move(*name), std::move(*pp.Parse(state)));
263+
}
264+
// expr-list
265+
case OmpTraitSelectorName::Value::Condition: // [6.0:321:33](*)
266+
case OmpTraitSelectorName::Value::Device_Num: { // [6.0:321:23-24](*)
267+
auto pp{propertyListParser(scalarExpr, extParser)};
268+
return OmpTraitSelector(std::move(*name), std::move(*pp.Parse(state)));
269+
}
270+
} // switch
271+
} else {
272+
// The other alternatives are `llvm::omp::Directive`, and `std::string`.
273+
// The former doesn't take any properties[1], the latter is a name of an
274+
// extension[2].
275+
// [1] [6.0:319:1-2]
276+
// [2] [6.0:319:36-37]
277+
auto pp{propertyListParser(extParser)};
278+
return OmpTraitSelector(std::move(*name), std::move(*pp.Parse(state)));
279+
}
280+
281+
llvm_unreachable("Unhandled trait name?");
282+
}
283+
284+
private:
285+
const Parser<OmpTraitSelectorName> np;
286+
};
209287

210-
TYPE_PARSER(sourced(construct<OmpTraitSelector>( //
211-
Parser<OmpTraitSelectorName>{}, //
212-
maybe(parenthesized(Parser<OmpTraitSelector::Properties>{})))))
288+
TYPE_PARSER(sourced(construct<OmpTraitSelector>(
289+
sourced(TraitSelectorParser(Parser<OmpTraitSelectorName>{})))))
213290

214291
TYPE_PARSER(construct<OmpTraitSetSelectorName::Value>(
215-
"CONSTRUCT" >> pure(OmpTraitSetSelectorName::Value::Construct) ||
216-
"DEVICE" >> pure(OmpTraitSetSelectorName::Value::Device) ||
217-
"IMPLEMENTATION" >> pure(OmpTraitSetSelectorName::Value::Implementation) ||
218-
"TARGET_DEVICE" >> pure(OmpTraitSetSelectorName::Value::Target_Device) ||
219-
"USER" >> pure(OmpTraitSetSelectorName::Value::User)))
292+
"CONSTRUCT"_id >> pure(OmpTraitSetSelectorName::Value::Construct) ||
293+
"DEVICE"_id >> pure(OmpTraitSetSelectorName::Value::Device) ||
294+
"IMPLEMENTATION"_id >>
295+
pure(OmpTraitSetSelectorName::Value::Implementation) ||
296+
"TARGET_DEVICE"_id >> pure(OmpTraitSetSelectorName::Value::Target_Device) ||
297+
"USER"_id >> pure(OmpTraitSetSelectorName::Value::User)))
220298

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

224302
TYPE_PARSER(sourced(construct<OmpTraitSetSelector>( //
225303
Parser<OmpTraitSetSelectorName>{},
226-
"=" >> braced(nonemptySeparated(Parser<OmpTraitSelector>{}, ","_tok)))))
304+
"=" >> braced(nonemptySeparated(Parser<OmpTraitSelector>{}, ",")))))
227305

228306
TYPE_PARSER(sourced(construct<OmpContextSelectorSpecification>(
229-
nonemptySeparated(Parser<OmpTraitSetSelector>{}, ","_tok))))
307+
nonemptySeparated(Parser<OmpTraitSetSelector>{}, ","))))
230308

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

flang/lib/Parser/unparse.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2079,10 +2079,11 @@ class UnparseVisitor {
20792079
Walk(x.v);
20802080
Put(")");
20812081
}
2082-
void Unparse(const OmpTraitPropertyExtension &x) {
2082+
void Unparse(const OmpTraitPropertyExtension::Complex &x) {
2083+
using PropList = std::list<common::Indirection<OmpTraitPropertyExtension>>;
20832084
Walk(std::get<OmpTraitPropertyName>(x.t));
20842085
Put("(");
2085-
Walk(std::get<OmpTraitPropertyExtension::ExtensionList>(x.t), ",");
2086+
Walk(std::get<PropList>(x.t), ",");
20862087
Put(")");
20872088
}
20882089
void Unparse(const OmpTraitSelector &x) {

0 commit comments

Comments
 (0)