[AsmParser] Rework logic around "region argument parsing"

[lattner]

Summary

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.

[lattner]

mlir/include/mlir/IR/OpImplementation.h

1126

incidentally, I think it makes sense to remove requiredOperandCount at this point. The original usecase was for things like binary operators that had two operands, but these get broken out into explicit parseOperand calls these days. This isn't related to this patch though.

[lattner]

Update flang

[rriddle]

I'm LGTM on the idea of removing the internal special casing, but the loss of parseRegionArgument as a user facing API leads to a loss in code readability IMO. I'd rather keep that API and properly forward to parseOperand.

mlir/include/mlir/IR/OpImplementation.h

1248

Can we keep retain these methods as a non-virtual API that clear forwards to parseOperand with the allowResultNumber flag set to false?

[lattner]

mlir/include/mlir/IR/OpImplementation.h

1248

Well sure, that's possible. I'm trying to do three things with this patch:

1. Fix a bug, implement checking that should have been there forever.
2. Reclaim the name "Argument": "Region arguments" as a thing doesn't really make sense as a name for this, and I'm going to be introducing a first class "Argument" concept which is different.
3. Reduce unneeded API surface area.

I agree that #3 isn't essential, but part of the reason that deleting it made sense to me is that the names were meaningless (regions don't have arguments, blocks do) and uncorrelated to the behavior (well, ok, there was no behavior before).

Is there a better name for what this is? Maybe renaming it would improve the situation. I still don't like having parallel parseRegionArgumentList versions, but that too could be improved by dropping the unused requiredOperandCount variant.

[rriddle]

mlir/include/mlir/IR/OpImplementation.h

1248

The problem with the patch as-is though, as it pertains to #3, is that the current parseRegionArgument* API is for a different class of things than parseOperand. They are generally used for the arguments of the entry block of a region (and in that sense the name RegionArgument does make sense, given that you pass the arguments when parsing a region; see parseRegion just above). It's essentially parsing a definition of a single SSA value. The exposure of allowResultNumber feels somewhat low level and obfuscates what the parser code is doing. "I'm parsing an argument to be passed to a region entry block" vs "I'm parsing an operand that can't have a result number"? The second one doesn't immediately click for me that its for a block argument.

[rriddle]

mlir/include/mlir/IR/OpImplementation.h

1248

What did you have in mind/intend for the "Argument" concept? I would expect that to subsume the RegionArgument stuff if it's intended for what the name suggests.

[lattner]

mlir/include/mlir/IR/OpImplementation.h

1248

Argument is the "ssa name + type + attr list + source loc" package. "RegionArgument" is just a limited form of SSA name. They're pretty different.

[rriddle]

mlir/include/mlir/IR/OpImplementation.h

1248

In what situations do you want to use that Argument concept?

[lattner]

River and I discussed this offline, I'm going to land this and build on it. Thx!

flang/lib/Optimizer/Dialect/FIROps.cpp

bodyRegion->front().addArguments(
       llvm::SmallVector<mlir::Location>(iterArgs.size(), result.location));
   result.addAttributes(attributes);
 }
 
 mlir::ParseResult fir::IterWhileOp::parse(mlir::OpAsmParser &parser,
                                           mlir::OperationState &result) {
   auto &builder = parser.getBuilder();
   mlir::OpAsmParser::UnresolvedOperand inductionVariable, lb, ub, step;
-  if (parser.parseLParen() || parser.parseRegionArgument(inductionVariable) ||
+  if (parser.parseLParen() ||
+      parser.parseOperand(inductionVariable, /*allowResultNumber=*/false) ||
       parser.parseEqual())
     return mlir::failure();
 
   // Parse loop bounds.
   auto indexType = builder.getIndexType();
   auto i1Type = builder.getIntegerType(1);
   if (parser.parseOperand(lb) ||
       parser.resolveOperand(lb, indexType, result.operands) ||
       parser.parseKeyword("to") || parser.parseOperand(ub) ||
       parser.resolveOperand(ub, indexType, result.operands) ||
       parser.parseKeyword("step") || parser.parseOperand(step) ||
       parser.parseRParen() ||
       parser.resolveOperand(step, indexType, result.operands))
     return mlir::failure();
 
   mlir::OpAsmParser::UnresolvedOperand iterateVar, iterateInput;
   if (parser.parseKeyword("and") || parser.parseLParen() ||
-      parser.parseRegionArgument(iterateVar) || parser.parseEqual() ||
-      parser.parseOperand(iterateInput) || parser.parseRParen() ||
+      parser.parseOperand(iterateVar, /*allowResultNumber=*/false) ||
+      parser.parseEqual() || parser.parseOperand(iterateInput) ||
+      parser.parseRParen() ||
       parser.resolveOperand(iterateInput, i1Type, result.operands))
     return mlir::failure();
 
   // Parse the initial iteration arguments.
   llvm::SmallVector<mlir::OpAsmParser::UnresolvedOperand> regionArgs;
   auto prependCount = false;
 
   // Induction variable.
void fir::DoLoopOp::build(mlir::OpBuilder &builder,
   result.addAttributes(attributes);
 }
 
 mlir::ParseResult fir::DoLoopOp::parse(mlir::OpAsmParser &parser,
                                        mlir::OperationState &result) {
   auto &builder = parser.getBuilder();
   mlir::OpAsmParser::UnresolvedOperand inductionVariable, lb, ub, step;
   // Parse the induction variable followed by '='.
-  if (parser.parseRegionArgument(inductionVariable) || parser.parseEqual())
+  if (parser.parseOperand(inductionVariable, /*allowResultNumber=*/false) ||
+      parser.parseEqual())
     return mlir::failure();
 
   // Parse loop bounds.
   auto indexType = builder.getIndexType();
   if (parser.parseOperand(lb) ||
       parser.resolveOperand(lb, indexType, result.operands) ||
       parser.parseKeyword("to") || parser.parseOperand(ub) ||
       parser.resolveOperand(ub, indexType, result.operands) ||

mlir/include/mlir/IR/OpImplementation.h

OptionalParseResult parseOptionalInteger(IntT &result) {
     result =
         (IntT)uintResult.sextOrTrunc(sizeof(IntT) * CHAR_BIT).getLimitedValue();
     if (APInt(uintResult.getBitWidth(), result) != uintResult)
       return emitError(loc, "integer value too large");
     return success();
   }
 
   /// These are the supported delimiters around operand lists and region
-  /// argument lists, used by parseOperandList and parseRegionArgumentList.
+  /// argument lists, used by parseOperandList.
   enum class Delimiter {
     /// Zero or more operands with no delimiters.
     None,
     /// Parens surrounding zero or more operands.
     Paren,
     /// Square brackets surrounding zero or more operands.
     Square,
     /// <> brackets surrounding zero or more operands.
virtual ParseResult parseGenericOperationAfterOpName(
       OperationState &result,
       Optional<ArrayRef<UnresolvedOperand>> parsedOperandType = llvm::None,
       Optional<ArrayRef<Block *>> parsedSuccessors = llvm::None,
       Optional<MutableArrayRef<std::unique_ptr<Region>>> parsedRegions =
           llvm::None,
       Optional<ArrayRef<NamedAttribute>> parsedAttributes = llvm::None,
       Optional<FunctionType> parsedFnType = llvm::None) = 0;
 
-  /// Parse a single operand.
-  virtual ParseResult parseOperand(UnresolvedOperand &result) = 0;
+  /// Parse a single SSA value operand name along with a result number if
+  /// `allowResultNumber` is true.
+  virtual ParseResult parseOperand(UnresolvedOperand &result,
+                                   bool allowResultNumber = true) = 0;
 
   /// Parse a single operand if present.
   virtual OptionalParseResult
-  parseOptionalOperand(UnresolvedOperand &result) = 0;
+  parseOptionalOperand(UnresolvedOperand &result,
+                       bool allowResultNumber = true) = 0;
 
   /// Parse zero or more SSA comma-separated operand references with a specified
   /// surrounding delimiter, and an optional required operand count.
-  virtual ParseResult
-  parseOperandList(SmallVectorImpl<UnresolvedOperand> &result,
-                   int requiredOperandCount = -1,
-                   Delimiter delimiter = Delimiter::None) = 0;
+  virtual ParseResult parseOperandList(
+      SmallVectorImpl<UnresolvedOperand> &result, int requiredOperandCount = -1,
+      Delimiter delimiter = Delimiter::None, bool allowResultNumber = true) = 0;
+
   ParseResult parseOperandList(SmallVectorImpl<UnresolvedOperand> &result,
-                               Delimiter delimiter) {
-    return parseOperandList(result, /*requiredOperandCount=*/-1, delimiter);
+                               Delimiter delimiter,
+                               bool allowResultNumber = true) {
+    return parseOperandList(result, /*requiredOperandCount=*/-1, delimiter,
+                            allowResultNumber);
   }
 
   /// Parse zero or more trailing SSA comma-separated trailing operand
   /// references with a specified surrounding delimiter, and an optional
   /// required operand count. A leading comma is expected before the operands.
   virtual ParseResult
   parseTrailingOperandList(SmallVectorImpl<UnresolvedOperand> &result,
                            int requiredOperandCount = -1,
public:
   /// allocated and placed in `region`. If no region is present or on failure,
   /// `region` remains untouched.
   virtual OptionalParseResult
   parseOptionalRegion(std::unique_ptr<Region> &region,
                       ArrayRef<UnresolvedOperand> arguments = {},
                       ArrayRef<Type> argTypes = {},
                       bool enableNameShadowing = false) = 0;
 
-  /// Parse a region argument, this argument is resolved when calling
-  /// 'parseRegion'.
-  virtual ParseResult parseRegionArgument(UnresolvedOperand &argument) = 0;
-
-  /// Parse zero or more region arguments with a specified surrounding
-  /// delimiter, and an optional required argument count. Region arguments
-  /// define new values; so this also checks if values with the same names have
-  /// not been defined yet.
-  virtual ParseResult
-  parseRegionArgumentList(SmallVectorImpl<UnresolvedOperand> &result,
-                          int requiredOperandCount = -1,
-                          Delimiter delimiter = Delimiter::None) = 0;
-  virtual ParseResult
-  parseRegionArgumentList(SmallVectorImpl<UnresolvedOperand> &result,
-                          Delimiter delimiter) {
-    return parseRegionArgumentList(result, /*requiredOperandCount=*/-1,
-                                   delimiter);
-  }
-
-  /// Parse a region argument if present.
-  virtual ParseResult
-  parseOptionalRegionArgument(UnresolvedOperand &argument) = 0;
-
   //===--------------------------------------------------------------------===//
   // Successor Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parse a single operation successor.
   virtual ParseResult parseSuccessor(Block *&dest) = 0;
 
   /// Parse an optional operation successor.

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

return p.emitError(
       p.getNameLoc(),
       "expected valid affine map representation for loop bounds");
 }
 
 ParseResult AffineForOp::parse(OpAsmParser &parser, OperationState &result) {
   auto &builder = parser.getBuilder();
   OpAsmParser::UnresolvedOperand inductionVariable;
   // Parse the induction variable followed by '='.
-  if (parser.parseRegionArgument(inductionVariable) || parser.parseEqual())
+  if (parser.parseOperand(inductionVariable, /*allowResultNumber=*/false) ||
+      parser.parseEqual())
     return failure();
 
   // Parse loop bounds.
   if (parseBound(/*isLower=*/true, result, parser) ||
       parser.parseKeyword("to", " between bounds") ||
       parseBound(/*isLower=*/false, result, parser))
     return failure();
 

 //               `to` parallel-bound steps? region attr-dict?
 // steps     ::= `steps` `(` integer-literals `)`
 //
 ParseResult AffineParallelOp::parse(OpAsmParser &parser,
                                     OperationState &result) {
   auto &builder = parser.getBuilder();
   auto indexType = builder.getIndexType();
   SmallVector<OpAsmParser::UnresolvedOperand, 4> ivs;
-  if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
-                                     OpAsmParser::Delimiter::Paren) ||
+  if (parser.parseOperandList(ivs, OpAsmParser::Delimiter::Paren,
+                              /*allowResultNumber=*/false) ||
       parser.parseEqual() ||
       parseAffineMapWithMinMax(parser, result, MinMaxKind::Max) ||
       parser.parseKeyword("to") ||
       parseAffineMapWithMinMax(parser, result, MinMaxKind::Min))
     return failure();
 
   AffineMapAttr stepsMapAttr;
   NamedAttrList stepsAttrs;

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

static ParseResult parseSwitchOpCases(
     DenseIntElementsAttr &caseValues,
     SmallVectorImpl<Block *> &caseDestinations,
     SmallVectorImpl<SmallVector<OpAsmParser::UnresolvedOperand>> &caseOperands,
     SmallVectorImpl<SmallVector<Type>> &caseOperandTypes) {
   if (parser.parseKeyword("default") || parser.parseColon() ||
       parser.parseSuccessor(defaultDestination))
     return failure();
   if (succeeded(parser.parseOptionalLParen())) {
-    if (parser.parseRegionArgumentList(defaultOperands) ||
+    if (parser.parseOperandList(defaultOperands, OpAsmParser::Delimiter::None,
+                                /*allowResultNumber=*/false) ||
         parser.parseColonTypeList(defaultOperandTypes) || parser.parseRParen())
       return failure();
   }
 
   SmallVector<APInt> values;
   unsigned bitWidth = flagType.getIntOrFloatBitWidth();
   while (succeeded(parser.parseOptionalComma())) {
     int64_t value = 0;
     if (failed(parser.parseInteger(value)))
       return failure();
     values.push_back(APInt(bitWidth, value));
 
     Block *destination;
     SmallVector<OpAsmParser::UnresolvedOperand> operands;
     SmallVector<Type> operandTypes;
     if (failed(parser.parseColon()) ||
         failed(parser.parseSuccessor(destination)))
       return failure();
     if (succeeded(parser.parseOptionalLParen())) {
-      if (failed(parser.parseRegionArgumentList(operands)) ||
+      if (failed(parser.parseOperandList(operands, OpAsmParser::Delimiter::None,
+                                         /*allowResultNumber=*/false)) ||
           failed(parser.parseColonTypeList(operandTypes)) ||
           failed(parser.parseRParen()))
         return failure();
     }
     caseDestinations.push_back(destination);
     caseOperands.emplace_back(operands);
     caseOperandTypes.emplace_back(operandTypes);
   }

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

 // SSA value uses.
 static ParseResult
 parseSizeAssignment(OpAsmParser &parser,
                     MutableArrayRef<OpAsmParser::UnresolvedOperand> sizes,
                     MutableArrayRef<OpAsmParser::UnresolvedOperand> regionSizes,
                     MutableArrayRef<OpAsmParser::UnresolvedOperand> indices) {
   assert(indices.size() == 3 && "space for three indices expected");
   SmallVector<OpAsmParser::UnresolvedOperand, 3> args;
-  if (parser.parseRegionArgumentList(args, /*requiredOperandCount=*/3,
-                                     OpAsmParser::Delimiter::Paren) ||
+  if (parser.parseOperandList(args, OpAsmParser::Delimiter::Paren,
+                              /*allowResultNumber=*/false) ||
       parser.parseKeyword("in") || parser.parseLParen())
     return failure();
   std::move(args.begin(), args.end(), indices.begin());
 
   for (int i = 0; i < 3; ++i) {
     if (i != 0 && parser.parseComma())
       return failure();
-    if (parser.parseRegionArgument(regionSizes[i]) || parser.parseEqual() ||
-        parser.parseOperand(sizes[i]))
+    if (parser.parseOperand(regionSizes[i], /*allowResultNumber=*/false) ||
+        parser.parseEqual() || parser.parseOperand(sizes[i]))
       return failure();
   }
 
   return parser.parseRParen();
 }
 
 /// Parses a Launch operation.
 /// operation ::= `gpu.launch` (`async` `[` ssa-id-list `]`)?
parseAttributions(OpAsmParser &parser, StringRef keyword,
   // Early exit for an empty list.
   if (succeeded(parser.parseOptionalRParen()))
     return success();
 
   do {
     OpAsmParser::UnresolvedOperand arg;
     Type type;
 
-    if (parser.parseRegionArgument(arg) || parser.parseColonType(type))
+    if (parser.parseOperand(arg, /*allowResultNumber=*/false) ||
+        parser.parseColonType(type))
       return failure();
 
     args.push_back(arg);
     argTypes.push_back(type);
   } while (succeeded(parser.parseOptionalComma()));
 
   return parser.parseRParen();
 }

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

do {
     values.push_back(APInt(bitWidth, value));
 
     Block *destination;
     SmallVector<OpAsmParser::UnresolvedOperand> operands;
     SmallVector<Type> operandTypes;
     if (parser.parseColon() || parser.parseSuccessor(destination))
       return failure();
     if (!parser.parseOptionalLParen()) {
-      if (parser.parseRegionArgumentList(operands) ||
+      if (parser.parseOperandList(operands, OpAsmParser::Delimiter::None,
+                                  /*allowResultNumber=*/false) ||
           parser.parseColonTypeList(operandTypes) || parser.parseRParen())
         return failure();
     }
     caseDestinations.push_back(destination);
     caseOperands.emplace_back(operands);
     caseOperandTypes.emplace_back(operandTypes);
   } while (!parser.parseOptionalComma());
 

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

parseOperandList(OpAsmParser &parser, StringRef keyword,
   // Exit early if the list is empty.
   if (succeeded(parser.parseOptionalRParen()))
     return success();
 
   do {
     OpAsmParser::UnresolvedOperand arg;
     Type type;
 
-    if (parser.parseRegionArgument(arg) || parser.parseColonType(type))
+    if (parser.parseOperand(arg, /*allowResultNumber=*/false) ||
+        parser.parseColonType(type))
       return failure();
 
     args.push_back(arg);
     argTypes.push_back(type);
   } while (succeeded(parser.parseOptionalComma()));
 
   if (failed(parser.parseRParen()))
     return failure();

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

 ParseResult
 parseWsLoopControl(OpAsmParser &parser, Region &region,
                    SmallVectorImpl<OpAsmParser::UnresolvedOperand> &lowerBound,
                    SmallVectorImpl<OpAsmParser::UnresolvedOperand> &upperBound,
                    SmallVectorImpl<OpAsmParser::UnresolvedOperand> &steps,
                    SmallVectorImpl<Type> &loopVarTypes, UnitAttr &inclusive) {
   // Parse an opening `(` followed by induction variables followed by `)`
   SmallVector<OpAsmParser::UnresolvedOperand> ivs;
-  if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
-                                     OpAsmParser::Delimiter::Paren))
+  if (parser.parseOperandList(ivs, OpAsmParser::Delimiter::Paren,
+                              /*allowResultNumber=*/false))
     return failure();
 
   size_t numIVs = ivs.size();
   Type loopVarType;
   if (parser.parseColonType(loopVarType))
     return failure();
 
   // Parse loop bounds.

 /// loop-control ::= `(` ssa-id-list `)` `:` type `=`  loop-bounds
 /// loop-bounds := `(` ssa-id-list `)` to `(` ssa-id-list `)` steps
 /// steps := `step` `(`ssa-id-list`)`
 /// clause-list ::= clause clause-list | empty
 /// clause ::= TODO
 ParseResult SimdLoopOp::parse(OpAsmParser &parser, OperationState &result) {
   // Parse an opening `(` followed by induction variables followed by `)`
   SmallVector<OpAsmParser::UnresolvedOperand> ivs;
-  if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
-                                     OpAsmParser::Delimiter::Paren))
+  if (parser.parseOperandList(ivs, OpAsmParser::Delimiter::Paren,
+                              /*allowResultNumber=*/false))
     return failure();
   int numIVs = static_cast<int>(ivs.size());
   Type loopVarType;
   if (parser.parseColonType(loopVarType))
     return failure();
   // Parse loop bounds.
   SmallVector<OpAsmParser::UnresolvedOperand> lower;
   if (parser.parseEqual() ||

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

state.regions.front()->addArgument(rangeType.getElementType(),
                                        state.location);
   }
 }
 
 ParseResult ForEachOp::parse(OpAsmParser &parser, OperationState &result) {
   // Parse the loop variable followed by type.
   OpAsmParser::UnresolvedOperand loopVariable;
   Type loopVariableType;
-  if (parser.parseRegionArgument(loopVariable) ||
+  if (parser.parseOperand(loopVariable, /*allowResultNumber=*/false) ||
       parser.parseColonType(loopVariableType))
     return failure();
 
   // Parse the "in" keyword.
   if (parser.parseKeyword("in", " after loop variable"))
     return failure();
 
   // Parse the operand (value range).

mlir/lib/Dialect/SCF/SCF.cpp

p.printRegion(getRegion(),
                 /*printBlockTerminators=*/hasIterOperands());
   p.printOptionalAttrDict((*this)->getAttrs());
 }
 
 ParseResult ForOp::parse(OpAsmParser &parser, OperationState &result) {
   auto &builder = parser.getBuilder();
   OpAsmParser::UnresolvedOperand inductionVariable, lb, ub, step;
   // Parse the induction variable followed by '='.
-  if (parser.parseRegionArgument(inductionVariable) || parser.parseEqual())
+  if (parser.parseOperand(inductionVariable, /*allowResultNumber=*/false) ||
+      parser.parseEqual())
     return failure();
 
   // Parse loop bounds.
   Type indexType = builder.getIndexType();
   if (parser.parseOperand(lb) ||
       parser.resolveOperand(lb, indexType, result.operands) ||
       parser.parseKeyword("to") || parser.parseOperand(ub) ||
       parser.resolveOperand(ub, indexType, result.operands) ||
LogicalResult ParallelOp::verify() {
   }
   return success();
 }
 
 ParseResult ParallelOp::parse(OpAsmParser &parser, OperationState &result) {
   auto &builder = parser.getBuilder();
   // Parse an opening `(` followed by induction variables followed by `)`
   SmallVector<OpAsmParser::UnresolvedOperand, 4> ivs;
-  if (parser.parseRegionArgumentList(ivs, /*requiredOperandCount=*/-1,
-                                     OpAsmParser::Delimiter::Paren))
+  if (parser.parseOperandList(ivs, OpAsmParser::Delimiter::Paren,
+                              /*allowResultNumber=*/false))
     return failure();
 
   // Parse loop bounds.
   SmallVector<OpAsmParser::UnresolvedOperand, 4> lower;
   if (parser.parseEqual() ||
       parser.parseOperandList(lower, ivs.size(),
                               OpAsmParser::Delimiter::Paren) ||
       parser.resolveOperands(lower, builder.getIndexType(), result.operands))

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

ParseResult WarpExecuteOnLane0Op::parse(OpAsmParser &parser,
   // Create the region.
   result.regions.reserve(1);
   Region *warpRegion = result.addRegion();
 
   auto &builder = parser.getBuilder();
   OpAsmParser::UnresolvedOperand laneId;
 
   // Parse predicate operand.
-  if (parser.parseLParen() || parser.parseRegionArgument(laneId) ||
+  if (parser.parseLParen() ||
+      parser.parseOperand(laneId, /*allowResultNumber=*/false) ||
       parser.parseRParen())
     return failure();
 
   int64_t warpSize;
   if (parser.parseLSquare() || parser.parseInteger(warpSize) ||
       parser.parseRSquare())
     return failure();
   result.addAttribute(getWarpSizeAttrName(OperationName(getOperationName(),

mlir/lib/IR/FunctionImplementation.cpp

ParseResult mlir::function_interface_impl::parseFunctionArgumentList(
   // types, or just be a type list.  It isn't ok to sometimes have SSA ID's and
   // sometimes not.
   auto parseArgument = [&]() -> ParseResult {
     SMLoc loc = parser.getCurrentLocation();
 
     // Parse argument name if present.
     OpAsmParser::UnresolvedOperand argument;
     Type argumentType;
-    if (succeeded(parser.parseOptionalRegionArgument(argument)) &&
-        !argument.name.empty()) {
+    auto hadSSAValue = parser.parseOptionalOperand(argument,
+                                                   /*allowResultNumber=*/false);
+    if (hadSSAValue.hasValue()) {
+      if (failed(hadSSAValue.getValue()))
+        return failure(); // Argument was present but malformed.
+
       // Reject this if the preceding argument was missing a name.
       if (argNames.empty() && !argTypes.empty())
         return parser.emitError(loc, "expected type instead of SSA identifier");
 
       // Parse required type.
       if (parser.parseColonType(argumentType))
         return failure();
     } else if (allowVariadic && succeeded(parser.parseOptionalEllipsis())) {

mlir/lib/Parser/Parser.cpp

public:
 
   /// Register a definition of a value with the symbol table.
   ParseResult addDefinition(UnresolvedOperand useInfo, Value value);
 
   /// Parse an optional list of SSA uses into 'results'.
   ParseResult
   parseOptionalSSAUseList(SmallVectorImpl<UnresolvedOperand> &results);
 
-  /// Parse a single SSA use into 'result'.
-  ParseResult parseSSAUse(UnresolvedOperand &result);
+  /// Parse a single SSA use into 'result'.  If 'allowResultNumber' is true then
+  /// we allow #42 syntax.
+  ParseResult parseSSAUse(UnresolvedOperand &result,
+                          bool allowResultNumber = true);
 
   /// Given a reference to an SSA value and its type, return a reference. This
   /// returns null on failure.
   Value resolveSSAUse(UnresolvedOperand useInfo, Type type);
 
   ParseResult parseSSADefOrUseAndType(
       function_ref<ParseResult(UnresolvedOperand, Type)> action);
 
return parseCommaSeparatedList([&]() -> ParseResult {
     return success();
   });
 }
 
 /// Parse a SSA operand for an operation.
 ///
 ///   ssa-use ::= ssa-id
 ///
-ParseResult OperationParser::parseSSAUse(UnresolvedOperand &result) {
+ParseResult OperationParser::parseSSAUse(UnresolvedOperand &result,
+                                         bool allowResultNumber) {
   result.name = getTokenSpelling();
   result.number = 0;
   result.location = getToken().getLoc();
   if (parseToken(Token::percent_identifier, "expected SSA operand"))
     return failure();
 
   // If we have an attribute ID, it is a result number.
   if (getToken().is(Token::hash_identifier)) {
+    if (!allowResultNumber)
+      return emitError("result number not allowed in argument list");
+
     if (auto value = getToken().getHashIdentifierNumber())
       result.number = value.getValue();
     else
       return emitError("invalid SSA value result number");
     consumeToken(Token::hash_identifier);
   }
 
   return success();
return AsmParserImpl<OpAsmParser>::emitError(loc, "custom op '" + opName +
                                                           "' " + message);
   }
 
   //===--------------------------------------------------------------------===//
   // Operand Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parse a single operand.
-  ParseResult parseOperand(UnresolvedOperand &result) override {
+  ParseResult parseOperand(UnresolvedOperand &result,
+                           bool allowResultNumber = true) override {
     OperationParser::UnresolvedOperand useInfo;
-    if (parser.parseSSAUse(useInfo))
+    if (parser.parseSSAUse(useInfo, allowResultNumber))
       return failure();
 
     result = {useInfo.location, useInfo.name, useInfo.number, {}};
 
     // Parse a source locator on the operand if present.
     return parseOptionalLocationSpecifier(result.sourceLoc);
   }
 
   /// Parse a single operand if present.
-  OptionalParseResult parseOptionalOperand(UnresolvedOperand &result) override {
+  OptionalParseResult
+  parseOptionalOperand(UnresolvedOperand &result,
+                       bool allowResultNumber = true) override {
     if (parser.getToken().is(Token::percent_identifier))
-      return parseOperand(result);
+      return parseOperand(result, allowResultNumber);
     return llvm::None;
   }
 
   /// Parse zero or more SSA comma-separated operand references with a specified
   /// surrounding delimiter, and an optional required operand count.
   ParseResult parseOperandList(SmallVectorImpl<UnresolvedOperand> &result,
                                int requiredOperandCount = -1,
-                               Delimiter delimiter = Delimiter::None) override {
-    return parseOperandOrRegionArgList(result, /*isOperandList=*/true,
-                                       requiredOperandCount, delimiter);
-  }
-
-  /// Parse zero or more SSA comma-separated operand or region arguments with
-  ///  optional surrounding delimiter and required operand count.
-  ParseResult
-  parseOperandOrRegionArgList(SmallVectorImpl<UnresolvedOperand> &result,
-                              bool isOperandList, int requiredOperandCount = -1,
-                              Delimiter delimiter = Delimiter::None) {
+                               Delimiter delimiter = Delimiter::None,
+                               bool allowResultNumber = true) override {
     auto startLoc = parser.getToken().getLoc();
 
     // The no-delimiter case has some special handling for better diagnostics.
     if (delimiter == Delimiter::None) {
       // parseCommaSeparatedList doesn't handle the missing case for "none",
       // so we handle it custom here.
       if (parser.getToken().isNot(Token::percent_identifier)) {
         // If we didn't require any operands or required exactly zero (weird)
         // then this is success.
         if (requiredOperandCount == -1 || requiredOperandCount == 0)
           return success();
 
         // Otherwise, try to produce a nice error message.
         if (parser.getToken().is(Token::l_paren) ||
             parser.getToken().is(Token::l_square))
           return emitError(startLoc, "unexpected delimiter");
         return emitError(startLoc, "invalid operand");
       }
     }
 
     auto parseOneOperand = [&]() -> ParseResult {
       UnresolvedOperand operandOrArg;
-      if (isOperandList ? parseOperand(operandOrArg)
-                        : parseRegionArgument(operandOrArg))
+      if (parseOperand(operandOrArg, allowResultNumber))
         return failure();
       result.push_back(operandOrArg);
       return success();
     };
 
     if (parseCommaSeparatedList(delimiter, parseOneOperand, " in operand list"))
       return failure();
 
OptionalParseResult parseOptionalRegion(
     std::unique_ptr<Region> newRegion = std::make_unique<Region>();
     if (parseRegion(*newRegion, arguments, argTypes, enableNameShadowing))
       return failure();
 
     region = std::move(newRegion);
     return success();
   }
 
-  /// Parse a region argument. The type of the argument will be resolved later
-  /// by a call to `parseRegion`.
-  ParseResult parseRegionArgument(UnresolvedOperand &argument) override {
-    return parseOperand(argument);
-  }
-
-  /// Parse a region argument if present.
-  ParseResult
-  parseOptionalRegionArgument(UnresolvedOperand &argument) override {
-    if (parser.getToken().isNot(Token::percent_identifier))
-      return success();
-    return parseRegionArgument(argument);
-  }
-
-  ParseResult
-  parseRegionArgumentList(SmallVectorImpl<UnresolvedOperand> &result,
-                          int requiredOperandCount = -1,
-                          Delimiter delimiter = Delimiter::None) override {
-    return parseOperandOrRegionArgList(result, /*isOperandList=*/false,
-                                       requiredOperandCount, delimiter);
-  }
-
   //===--------------------------------------------------------------------===//
   // Successor Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parse a single operation successor.
   ParseResult parseSuccessor(Block *&dest) override {
     return parser.parseSuccessor(dest);
   }
public:
   OptionalParseResult parseOptionalAssignmentList(
       SmallVectorImpl<UnresolvedOperand> &lhs,
       SmallVectorImpl<UnresolvedOperand> &rhs) override {
     if (failed(parseOptionalLParen()))
       return llvm::None;
 
     auto parseElt = [&]() -> ParseResult {
       UnresolvedOperand regionArg, operand;
-      if (parseRegionArgument(regionArg) || parseEqual() ||
-          parseOperand(operand))
+      if (parseOperand(regionArg, /*allowResultNumber=*/false) ||
+          parseEqual() || parseOperand(operand))
         return failure();
       lhs.push_back(regionArg);
       rhs.push_back(operand);
       return success();
     };
     return parser.parseCommaSeparatedListUntil(Token::r_paren, parseElt);
   }
 
   /// Parse a list of assignments of the form
   ///   (%x1 = %y1 : type1, %x2 = %y2 : type2, ...).
   OptionalParseResult
   parseOptionalAssignmentListWithTypes(SmallVectorImpl<UnresolvedOperand> &lhs,
                                        SmallVectorImpl<UnresolvedOperand> &rhs,
                                        SmallVectorImpl<Type> &types) override {
     if (failed(parseOptionalLParen()))
       return llvm::None;
 
     auto parseElt = [&]() -> ParseResult {
       UnresolvedOperand regionArg, operand;
       Type type;
-      if (parseRegionArgument(regionArg) || parseEqual() ||
-          parseOperand(operand) || parseColon() || parseType(type))
+      if (parseOperand(regionArg, /*allowResultNumber=*/false) ||
+          parseEqual() || parseOperand(operand) || parseColon() ||
+          parseType(type))
         return failure();
       lhs.push_back(regionArg);
       rhs.push_back(operand);
       types.push_back(type);
       return success();
     };
     return parser.parseCommaSeparatedListUntil(Token::r_paren, parseElt);
   }

mlir/test/Dialect/Affine/invalid.mlir

func.func @affine_for_iter_args_mismatch(%buffer: memref<1024xf32>) -> f32 {
   %sum_0 = arith.constant 0.0 : f32
   // expected-error@+1 {{mismatch between the number of loop-carried values and results}}
   %res = affine.for %i = 0 to 10 step 2 iter_args(%sum_iter = %sum_0) -> (f32, f32) {
     %t = affine.load %buffer[%i] : memref<1024xf32>
     affine.yield %t : f32
   }
   return %res : f32
 }
+
+
+// -----
+
+func.func @result_number() {
+  // expected-error@+1 {{result number not allowed}}
+  affine.for %n0#0 = 0 to 7 {
+  }
+  return
+}

mlir/test/lib/Dialect/Test/TestDialect.cpp

 
 //===----------------------------------------------------------------------===//
 // Test PolyForOp - parse list of region arguments.
 //===----------------------------------------------------------------------===//
 
 ParseResult PolyForOp::parse(OpAsmParser &parser, OperationState &result) {
   SmallVector<OpAsmParser::UnresolvedOperand, 4> ivsInfo;
   // Parse list of region arguments without a delimiter.
-  if (parser.parseRegionArgumentList(ivsInfo))
+  if (parser.parseOperandList(ivsInfo, OpAsmParser::Delimiter::None,
+                              /*allowResultNumber=*/false))
     return failure();
 
   // Parse the body region.
   Region *body = result.addRegion();
   auto &builder = parser.getBuilder();
   SmallVector<Type, 4> argTypes(ivsInfo.size(), builder.getIndexType());
   return parser.parseRegion(*body, ivsInfo, argTypes);
 }