Skip to content

Commit 5dedf91

Browse files
lattnerlattner
committed
[AsmParser] Rework logic around "region argument parsing"
The asm parser had a notional distinction between parsing an operand (like "%foo" or "%4#3") and parsing a region argument (which isn't supposed to allow a result number like #3). Unfortunately the implementation has two problems: 1) It didn't actually check for the result number and reject it. parseRegionArgument and parseOperand were identical. 2) It had a lot of machinery built up around it that paralleled operand parsing. This also was functionally identical, but also had some subtle differences (e.g. the parseOptional stuff had a different result type). I thought about just removing all of this, but decided that the missing error checking was important, so I reimplemented it with a `allowResultNumber` flag on parseOperand. This keeps the codepaths unified and adds the missing error checks. Differential Revision: https://reviews.llvm.org/D124470
1 parent 6c81b57 commit 5dedf91

File tree

15 files changed

+93
-107
lines changed

15 files changed

+93
-107
lines changed

‎flang/lib/Optimizer/Dialect/FIROps.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1563,7 +1563,8 @@ mlir::ParseResult fir::IterWhileOp::parse(mlir::OpAsmParser &parser,
15631563
mlir::OperationState &result) {
15641564
auto &builder = parser.getBuilder();
15651565
mlir::OpAsmParser::UnresolvedOperand inductionVariable, lb, ub, step;
1566-
if (parser.parseLParen() || parser.parseRegionArgument(inductionVariable) ||
1566+
if (parser.parseLParen() ||
1567+
parser.parseOperand(inductionVariable, /*allowResultNumber=*/false) ||
15671568
parser.parseEqual())
15681569
return mlir::failure();
15691570

@@ -1581,8 +1582,9 @@ mlir::ParseResult fir::IterWhileOp::parse(mlir::OpAsmParser &parser,
15811582

15821583
mlir::OpAsmParser::UnresolvedOperand iterateVar, iterateInput;
15831584
if (parser.parseKeyword("and") || parser.parseLParen() ||
1584-
parser.parseRegionArgument(iterateVar) || parser.parseEqual() ||
1585-
parser.parseOperand(iterateInput) || parser.parseRParen() ||
1585+
parser.parseOperand(iterateVar, /*allowResultNumber=*/false) ||
1586+
parser.parseEqual() || parser.parseOperand(iterateInput) ||
1587+
parser.parseRParen() ||
15861588
parser.resolveOperand(iterateInput, i1Type, result.operands))
15871589
return mlir::failure();
15881590

@@ -1876,7 +1878,8 @@ mlir::ParseResult fir::DoLoopOp::parse(mlir::OpAsmParser &parser,
18761878
auto &builder = parser.getBuilder();
18771879
mlir::OpAsmParser::UnresolvedOperand inductionVariable, lb, ub, step;
18781880
// Parse the induction variable followed by '='.
1879-
if (parser.parseRegionArgument(inductionVariable) || parser.parseEqual())
1881+
if (parser.parseOperand(inductionVariable, /*allowResultNumber=*/false) ||
1882+
parser.parseEqual())
18801883
return mlir::failure();
18811884

18821885
// Parse loop bounds.

‎mlir/include/mlir/IR/OpImplementation.h

Lines changed: 15 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -584,7 +584,7 @@ class AsmParser {
584584
}
585585

586586
/// These are the supported delimiters around operand lists and region
587-
/// argument lists, used by parseOperandList and parseRegionArgumentList.
587+
/// argument lists, used by parseOperandList.
588588
enum class Delimiter {
589589
/// Zero or more operands with no delimiters.
590590
None,
@@ -1110,22 +1110,27 @@ class OpAsmParser : public AsmParser {
11101110
Optional<ArrayRef<NamedAttribute>> parsedAttributes = llvm::None,
11111111
Optional<FunctionType> parsedFnType = llvm::None) = 0;
11121112

1113-
/// Parse a single operand.
1114-
virtual ParseResult parseOperand(UnresolvedOperand &result) = 0;
1113+
/// Parse a single SSA value operand name along with a result number if
1114+
/// `allowResultNumber` is true.
1115+
virtual ParseResult parseOperand(UnresolvedOperand &result,
1116+
bool allowResultNumber = true) = 0;
11151117

11161118
/// Parse a single operand if present.
11171119
virtual OptionalParseResult
1118-
parseOptionalOperand(UnresolvedOperand &result) = 0;
1120+
parseOptionalOperand(UnresolvedOperand &result,
1121+
bool allowResultNumber = true) = 0;
11191122

11201123
/// Parse zero or more SSA comma-separated operand references with a specified
11211124
/// surrounding delimiter, and an optional required operand count.
1122-
virtual ParseResult
1123-
parseOperandList(SmallVectorImpl<UnresolvedOperand> &result,
1124-
int requiredOperandCount = -1,
1125-
Delimiter delimiter = Delimiter::None) = 0;
1125+
virtual ParseResult parseOperandList(
1126+
SmallVectorImpl<UnresolvedOperand> &result, int requiredOperandCount = -1,
1127+
Delimiter delimiter = Delimiter::None, bool allowResultNumber = true) = 0;
1128+
11261129
ParseResult parseOperandList(SmallVectorImpl<UnresolvedOperand> &result,
1127-
Delimiter delimiter) {
1128-
return parseOperandList(result, /*requiredOperandCount=*/-1, delimiter);
1130+
Delimiter delimiter,
1131+
bool allowResultNumber = true) {
1132+
return parseOperandList(result, /*requiredOperandCount=*/-1, delimiter,
1133+
allowResultNumber);
11291134
}
11301135

11311136
/// Parse zero or more trailing SSA comma-separated trailing operand
@@ -1243,29 +1248,6 @@ class OpAsmParser : public AsmParser {
12431248
ArrayRef<Type> argTypes = {},
12441249
bool enableNameShadowing = false) = 0;
12451250

1246-
/// Parse a region argument, this argument is resolved when calling
1247-
/// 'parseRegion'.
1248-
virtual ParseResult parseRegionArgument(UnresolvedOperand &argument) = 0;
1249-
1250-
/// Parse zero or more region arguments with a specified surrounding
1251-
/// delimiter, and an optional required argument count. Region arguments
1252-
/// define new values; so this also checks if values with the same names have
1253-
/// not been defined yet.
1254-
virtual ParseResult
1255-
parseRegionArgumentList(SmallVectorImpl<UnresolvedOperand> &result,
1256-
int requiredOperandCount = -1,
1257-
Delimiter delimiter = Delimiter::None) = 0;
1258-
virtual ParseResult
1259-
parseRegionArgumentList(SmallVectorImpl<UnresolvedOperand> &result,
1260-
Delimiter delimiter) {
1261-
return parseRegionArgumentList(result, /*requiredOperandCount=*/-1,
1262-
delimiter);
1263-
}
1264-
1265-
/// Parse a region argument if present.
1266-
virtual ParseResult
1267-
parseOptionalRegionArgument(UnresolvedOperand &argument) = 0;
1268-
12691251
//===--------------------------------------------------------------------===//
12701252
// Successor Parsing
12711253
//===--------------------------------------------------------------------===//

‎mlir/lib/Dialect/Affine/IR/AffineOps.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1433,7 +1433,8 @@ ParseResult AffineForOp::parse(OpAsmParser &parser, OperationState &result) {
14331433
auto &builder = parser.getBuilder();
14341434
OpAsmParser::UnresolvedOperand inductionVariable;
14351435
// Parse the induction variable followed by '='.
1436-
if (parser.parseRegionArgument(inductionVariable) || parser.parseEqual())
1436+
if (parser.parseOperand(inductionVariable, /*allowResultNumber=*/false) ||
1437+
parser.parseEqual())
14371438
return failure();
14381439

14391440
// Parse loop bounds.
@@ -3527,8 +3528,8 @@ ParseResult AffineParallelOp::parse(OpAsmParser &parser,
35273528
auto &builder = parser.getBuilder();
35283529
auto indexType = builder.getIndexType();
35293530
SmallVector<OpAsmParser::UnresolvedOperand, 4> ivs;
3530-
if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
3531-
OpAsmParser::Delimiter::Paren) ||
3531+
if (parser.parseOperandList(ivs, OpAsmParser::Delimiter::Paren,
3532+
/*allowResultNumber=*/false) ||
35323533
parser.parseEqual() ||
35333534
parseAffineMapWithMinMax(parser, result, MinMaxKind::Max) ||
35343535
parser.parseKeyword("to") ||

‎mlir/lib/Dialect/ControlFlow/IR/ControlFlowOps.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -489,7 +489,8 @@ static ParseResult parseSwitchOpCases(
489489
parser.parseSuccessor(defaultDestination))
490490
return failure();
491491
if (succeeded(parser.parseOptionalLParen())) {
492-
if (parser.parseRegionArgumentList(defaultOperands) ||
492+
if (parser.parseOperandList(defaultOperands, OpAsmParser::Delimiter::None,
493+
/*allowResultNumber=*/false) ||
493494
parser.parseColonTypeList(defaultOperandTypes) || parser.parseRParen())
494495
return failure();
495496
}
@@ -509,7 +510,8 @@ static ParseResult parseSwitchOpCases(
509510
failed(parser.parseSuccessor(destination)))
510511
return failure();
511512
if (succeeded(parser.parseOptionalLParen())) {
512-
if (failed(parser.parseRegionArgumentList(operands)) ||
513+
if (failed(parser.parseOperandList(operands, OpAsmParser::Delimiter::None,
514+
/*allowResultNumber=*/false)) ||
513515
failed(parser.parseColonTypeList(operandTypes)) ||
514516
failed(parser.parseRParen()))
515517
return failure();

‎mlir/lib/Dialect/GPU/IR/GPUDialect.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -539,17 +539,17 @@ parseSizeAssignment(OpAsmParser &parser,
539539
MutableArrayRef<OpAsmParser::UnresolvedOperand> indices) {
540540
assert(indices.size() == 3 && "space for three indices expected");
541541
SmallVector<OpAsmParser::UnresolvedOperand, 3> args;
542-
if (parser.parseRegionArgumentList(args, /*requiredOperandCount=*/3,
543-
OpAsmParser::Delimiter::Paren) ||
542+
if (parser.parseOperandList(args, OpAsmParser::Delimiter::Paren,
543+
/*allowResultNumber=*/false) ||
544544
parser.parseKeyword("in") || parser.parseLParen())
545545
return failure();
546546
std::move(args.begin(), args.end(), indices.begin());
547547

548548
for (int i = 0; i < 3; ++i) {
549549
if (i != 0 && parser.parseComma())
550550
return failure();
551-
if (parser.parseRegionArgument(regionSizes[i]) || parser.parseEqual() ||
552-
parser.parseOperand(sizes[i]))
551+
if (parser.parseOperand(regionSizes[i], /*allowResultNumber=*/false) ||
552+
parser.parseEqual() || parser.parseOperand(sizes[i]))
553553
return failure();
554554
}
555555

@@ -869,7 +869,8 @@ parseAttributions(OpAsmParser &parser, StringRef keyword,
869869
OpAsmParser::UnresolvedOperand arg;
870870
Type type;
871871

872-
if (parser.parseRegionArgument(arg) || parser.parseColonType(type))
872+
if (parser.parseOperand(arg, /*allowResultNumber=*/false) ||
873+
parser.parseColonType(type))
873874
return failure();
874875

875876
args.push_back(arg);

‎mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,8 @@ static ParseResult parseSwitchOpCases(
332332
if (parser.parseColon() || parser.parseSuccessor(destination))
333333
return failure();
334334
if (!parser.parseOptionalLParen()) {
335-
if (parser.parseRegionArgumentList(operands) ||
335+
if (parser.parseOperandList(operands, OpAsmParser::Delimiter::None,
336+
/*allowResultNumber=*/false) ||
336337
parser.parseColonTypeList(operandTypes) || parser.parseRParen())
337338
return failure();
338339
}

‎mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,8 @@ parseOperandList(OpAsmParser &parser, StringRef keyword,
7070
OpAsmParser::UnresolvedOperand arg;
7171
Type type;
7272

73-
if (parser.parseRegionArgument(arg) || parser.parseColonType(type))
73+
if (parser.parseOperand(arg, /*allowResultNumber=*/false) ||
74+
parser.parseColonType(type))
7475
return failure();
7576

7677
args.push_back(arg);

‎mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -524,8 +524,8 @@ parseWsLoopControl(OpAsmParser &parser, Region &region,
524524
SmallVectorImpl<Type> &loopVarTypes, UnitAttr &inclusive) {
525525
// Parse an opening `(` followed by induction variables followed by `)`
526526
SmallVector<OpAsmParser::UnresolvedOperand> ivs;
527-
if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
528-
OpAsmParser::Delimiter::Paren))
527+
if (parser.parseOperandList(ivs, OpAsmParser::Delimiter::Paren,
528+
/*allowResultNumber=*/false))
529529
return failure();
530530

531531
size_t numIVs = ivs.size();
@@ -587,8 +587,8 @@ void printWsLoopControl(OpAsmPrinter &p, Operation *op, Region &region,
587587
ParseResult SimdLoopOp::parse(OpAsmParser &parser, OperationState &result) {
588588
// Parse an opening `(` followed by induction variables followed by `)`
589589
SmallVector<OpAsmParser::UnresolvedOperand> ivs;
590-
if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
591-
OpAsmParser::Delimiter::Paren))
590+
if (parser.parseOperandList(ivs, OpAsmParser::Delimiter::Paren,
591+
/*allowResultNumber=*/false))
592592
return failure();
593593
int numIVs = static_cast<int>(ivs.size());
594594
Type loopVarType;

‎mlir/lib/Dialect/PDLInterp/IR/PDLInterp.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ ParseResult ForEachOp::parse(OpAsmParser &parser, OperationState &result) {
103103
// Parse the loop variable followed by type.
104104
OpAsmParser::UnresolvedOperand loopVariable;
105105
Type loopVariableType;
106-
if (parser.parseRegionArgument(loopVariable) ||
106+
if (parser.parseOperand(loopVariable, /*allowResultNumber=*/false) ||
107107
parser.parseColonType(loopVariableType))
108108
return failure();
109109

‎mlir/lib/Dialect/SCF/SCF.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -401,7 +401,8 @@ ParseResult ForOp::parse(OpAsmParser &parser, OperationState &result) {
401401
auto &builder = parser.getBuilder();
402402
OpAsmParser::UnresolvedOperand inductionVariable, lb, ub, step;
403403
// Parse the induction variable followed by '='.
404-
if (parser.parseRegionArgument(inductionVariable) || parser.parseEqual())
404+
if (parser.parseOperand(inductionVariable, /*allowResultNumber=*/false) ||
405+
parser.parseEqual())
405406
return failure();
406407

407408
// Parse loop bounds.
@@ -1975,8 +1976,8 @@ ParseResult ParallelOp::parse(OpAsmParser &parser, OperationState &result) {
19751976
auto &builder = parser.getBuilder();
19761977
// Parse an opening `(` followed by induction variables followed by `)`
19771978
SmallVector<OpAsmParser::UnresolvedOperand, 4> ivs;
1978-
if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
1979-
OpAsmParser::Delimiter::Paren))
1979+
if (parser.parseOperandList(ivs, OpAsmParser::Delimiter::Paren,
1980+
/*allowResultNumber=*/false))
19801981
return failure();
19811982

19821983
// Parse loop bounds.

‎mlir/lib/Dialect/Vector/IR/VectorOps.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4698,7 +4698,8 @@ ParseResult WarpExecuteOnLane0Op::parse(OpAsmParser &parser,
46984698
OpAsmParser::UnresolvedOperand laneId;
46994699

47004700
// Parse predicate operand.
4701-
if (parser.parseLParen() || parser.parseRegionArgument(laneId) ||
4701+
if (parser.parseLParen() ||
4702+
parser.parseOperand(laneId, /*allowResultNumber=*/false) ||
47024703
parser.parseRParen())
47034704
return failure();
47044705

‎mlir/lib/IR/FunctionImplementation.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,12 @@ ParseResult mlir::function_interface_impl::parseFunctionArgumentList(
3030
// Parse argument name if present.
3131
OpAsmParser::UnresolvedOperand argument;
3232
Type argumentType;
33-
if (succeeded(parser.parseOptionalRegionArgument(argument)) &&
34-
!argument.name.empty()) {
33+
auto hadSSAValue = parser.parseOptionalOperand(argument,
34+
/*allowResultNumber=*/false);
35+
if (hadSSAValue.hasValue()) {
36+
if (failed(hadSSAValue.getValue()))
37+
return failure(); // Argument was present but malformed.
38+
3539
// Reject this if the preceding argument was missing a name.
3640
if (argNames.empty() && !argTypes.empty())
3741
return parser.emitError(loc, "expected type instead of SSA identifier");

0 commit comments

Comments
 (0)