[AsmParser/Printer] Rework sourceloc support for function arguments.

[lattner]

Summary

When Location tracking support for block arguments was added, we
discussed various approaches to threading support for this through
function-like argument parsing. At the time, we added a parallel array
of locations that could hold this. It turns out that that approach was
verbose and error prone, roughly no one adopted it.

This patch takes a different approach, adding an optional source
locator to the UnresolvedOperand class. This fits much more naturally
into the standard structure we use for representing locators, and gives
all the function like dialects locator support for free (e.g. see the
test adding an example for the LLVM dialect).

[lattner]

We discussed this in some other patch that I lost track of, this was just something on my deep todo list.

[rriddle]

Much much better, thanks for doing this!

mlir/lib/Parser/Parser.cpp

1276

[lattner]

Add missing blank line

[lattner]

Thank you for the quick review!

[mehdi_amini]

mlir/lib/Parser/Parser.cpp

1278

We've got internal report that now "foo %x loc(...) now assigns the loc to %x instead of foo" and it breaks custom parsers, thoughts?

[lattner]

mlir/lib/Parser/Parser.cpp

1278

No good deed goes unpunished :-) I'll look into it!

[lattner]

mlir/lib/Parser/Parser.cpp

1278

WDYT about this approach (cc @rriddle):

1. Revert parseOperand back to not parsing the location information. Doing so will cause a bunch of ambiguities like this because we parse "%foo" in lots of contexts.
2. Introduce a new "parseArgument" parser hook that parses the typical %42 : loc(...) type { attrs } grammar used by function-signature-like things.
3. parseArgument would fill in a new ResolvedArgument struct, instead of UnresolvedOperand since it has the type it can resolve, and this will also hold the attributes and source loc.

I think this will restore compatibility, capture an important and pervasive pattern with a first class API that we always reimplement all over the place, and would allow us to add ODS support for this pattern in a nice way (but I'm not volunteering to do that, I don't know ODS internals well enough).

WDYT? If you both find this approach to be promising, then I'll try to get a PR up tomorrow.

Mehdi if this patch is causing a problem for you, please feel free to revert while we figure this out.

[rriddle]

mlir/lib/Parser/Parser.cpp

1278

The overall approach SGTM.

[csigg]

mlir/lib/Parser/Parser.cpp

1278

Mehdi if this patch is causing a problem for you, please feel free to revert while we figure this out.

No need to revert. I think the only test that started failing is this one and I just switched to -mlir-print-op-generic to work around it for now.

[lattner]

Sweet, I'll get on this in a few minutes, thx!

mlir/include/mlir/IR/FunctionImplementation.h

 /// Parses function arguments using `parser`. The `allowVariadic` argument
 /// indicates whether functions with variadic arguments are supported. The
 /// trailing arguments are populated by this function with names, types,
 /// attributes and locations of the arguments.
 ParseResult parseFunctionArgumentList(
     OpAsmParser &parser, bool allowAttributes, bool allowVariadic,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &argNames,
     SmallVectorImpl<Type> &argTypes, SmallVectorImpl<NamedAttrList> &argAttrs,
-    SmallVectorImpl<Location> &argLocations, bool &isVariadic);
+    bool &isVariadic);
 
 /// Parses a function signature using `parser`. The `allowVariadic` argument
 /// indicates whether functions with variadic arguments are supported. The
 /// trailing arguments are populated by this function with names, types,
 /// attributes and locations of the arguments and those of the results.
 ParseResult parseFunctionSignature(
     OpAsmParser &parser, bool allowVariadic,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &argNames,
     SmallVectorImpl<Type> &argTypes, SmallVectorImpl<NamedAttrList> &argAttrs,
-    SmallVectorImpl<Location> &argLocations, bool &isVariadic,
-    SmallVectorImpl<Type> &resultTypes,
+    bool &isVariadic, SmallVectorImpl<Type> &resultTypes,
     SmallVectorImpl<NamedAttrList> &resultAttrs);
 
 /// Parser implementation for function-like operations.  Uses
 /// `funcTypeBuilder` to construct the custom function type given lists of
 /// input and output types.  If `allowVariadic` is set, the parser will accept
 /// trailing ellipsis in the function signature and indicate to the builder
 /// whether the function is variadic.  If the builder returns a null type,
 /// `result` will not contain the `type` attribute.  The caller can then add a

mlir/include/mlir/IR/OpImplementation.h

public:
   // Operand Parsing
   //===--------------------------------------------------------------------===//
 
   /// This is the representation of an operand reference.
   struct UnresolvedOperand {
     SMLoc location;  // Location of the token.
     StringRef name;  // Value name, e.g. %42 or %abc
     unsigned number; // Number, e.g. 12 for an operand like %xyz#12
+    Optional<Location> sourceLoc; // Source location specifier if present.
   };
 
   /// Parse different components, viz., use-info of operand(s), successor(s),
   /// region(s), attribute(s) and function-type, of the generic form of an
   /// operation instance and populate the input operation-state 'result' with
   /// those components. If any of the components is explicitly provided, then
   /// skip parsing that component.
   virtual ParseResult parseGenericOperationAfterOpName(
parseAffineExprOfSSAIds(SmallVectorImpl<UnresolvedOperand> &dimOperands,
                           AffineExpr &expr) = 0;
 
   //===--------------------------------------------------------------------===//
   // Region Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parses a region. Any parsed blocks are appended to 'region' and must be
   /// moved to the op regions after the op is created. The first block of the
-  /// region takes 'arguments' of types 'argTypes'. If `argLocations` is
-  /// non-empty it contains a location to be attached to each argument. If
-  /// 'enableNameShadowing' is set to true, the argument names are allowed to
-  /// shadow the names of other existing SSA values defined above the region
-  /// scope. 'enableNameShadowing' can only be set to true for regions attached
-  /// to operations that are 'IsolatedFromAbove'.
+  /// region takes 'arguments' of types 'argTypes'.  If 'enableNameShadowing' is
+  /// set to true, the argument names are allowed to shadow the names of other
+  /// existing SSA values defined above the region scope. 'enableNameShadowing'
+  /// can only be set to true for regions attached to operations that are
+  /// 'IsolatedFromAbove'.
   virtual ParseResult parseRegion(Region &region,
                                   ArrayRef<UnresolvedOperand> arguments = {},
                                   ArrayRef<Type> argTypes = {},
-                                  ArrayRef<Location> argLocations = {},
                                   bool enableNameShadowing = false) = 0;
 
   /// Parses a region if present.
   virtual OptionalParseResult parseOptionalRegion(
       Region &region, ArrayRef<UnresolvedOperand> arguments = {},
-      ArrayRef<Type> argTypes = {}, ArrayRef<Location> argLocations = {},
-      bool enableNameShadowing = false) = 0;
+      ArrayRef<Type> argTypes = {}, bool enableNameShadowing = false) = 0;
 
   /// Parses a region if present. If the region is present, a new region is
   /// 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 = {},

mlir/lib/Dialect/Async/IR/Async.cpp

ParseResult ExecuteOp::parse(OpAsmParser &parser, OperationState &result) {
   if (parser.parseOptionalAttrDictWithKeyword(attrs))
     return failure();
   result.addAttributes(attrs);
 
   // Parse asynchronous region.
   Region *body = result.addRegion();
   if (parser.parseRegion(*body, /*arguments=*/{unwrappedArgs},
                          /*argTypes=*/{unwrappedTypes},
-                         /*argLocations=*/{},
                          /*enableNameShadowing=*/false))
     return failure();
 
   return success();
 }
 
 LogicalResult ExecuteOp::verifyRegions() {
   // Unwrap async.execute value operands types.

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

 
 static ParseResult parseLaunchFuncOperands(
     OpAsmParser &parser,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &argNames,
     SmallVectorImpl<Type> &argTypes) {
   if (parser.parseOptionalKeyword("args"))
     return success();
   SmallVector<NamedAttrList> argAttrs;
-  SmallVector<Location> argLocations;
   bool isVariadic = false;
   return function_interface_impl::parseFunctionArgumentList(
       parser, /*allowAttributes=*/false,
-      /*allowVariadic=*/false, argNames, argTypes, argAttrs, argLocations,
-      isVariadic);
+      /*allowVariadic=*/false, argNames, argTypes, argAttrs, isVariadic);
 }
 
 static void printLaunchFuncOperands(OpAsmPrinter &printer, Operation *,
                                     OperandRange operands, TypeRange types) {
   if (operands.empty())
     return;
   printer << "args(";
   llvm::interleaveComma(llvm::zip(operands, types), printer,

 ///                 (`->` function-result-list)? memory-attribution `kernel`?
 ///                 function-attributes? region
 ParseResult GPUFuncOp::parse(OpAsmParser &parser, OperationState &result) {
   SmallVector<OpAsmParser::UnresolvedOperand> entryArgs;
   SmallVector<NamedAttrList> argAttrs;
   SmallVector<NamedAttrList> resultAttrs;
   SmallVector<Type> argTypes;
   SmallVector<Type> resultTypes;
-  SmallVector<Location> argLocations;
   bool isVariadic;
 
   // Parse the function name.
   StringAttr nameAttr;
   if (parser.parseSymbolName(nameAttr, ::mlir::SymbolTable::getSymbolAttrName(),
                              result.attributes))
     return failure();
 
   auto signatureLocation = parser.getCurrentLocation();
   if (failed(function_interface_impl::parseFunctionSignature(
           parser, /*allowVariadic=*/false, entryArgs, argTypes, argAttrs,
-          argLocations, isVariadic, resultTypes, resultAttrs)))
+          isVariadic, resultTypes, resultAttrs)))
     return failure();
 
   if (entryArgs.empty() && !argTypes.empty())
     return parser.emitError(signatureLocation)
            << "gpu.func requires named arguments";
 
   // Construct the function type. More types will be added to the region, but
   // not to the function type.

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

result.addAttribute(
                            parser, result, LLVM::Linkage::External)));
 
   StringAttr nameAttr;
   SmallVector<OpAsmParser::UnresolvedOperand> entryArgs;
   SmallVector<NamedAttrList> argAttrs;
   SmallVector<NamedAttrList> resultAttrs;
   SmallVector<Type> argTypes;
   SmallVector<Type> resultTypes;
-  SmallVector<Location> argLocations;
   bool isVariadic;
 
   auto signatureLocation = parser.getCurrentLocation();
   if (parser.parseSymbolName(nameAttr, SymbolTable::getSymbolAttrName(),
                              result.attributes) ||
       function_interface_impl::parseFunctionSignature(
           parser, /*allowVariadic=*/true, entryArgs, argTypes, argAttrs,
-          argLocations, isVariadic, resultTypes, resultAttrs))
+          isVariadic, resultTypes, resultAttrs))
     return failure();
 
   auto type =
       buildLLVMFunctionType(parser, signatureLocation, argTypes, resultTypes,
                             function_interface_impl::VariadicFlag(isVariadic));
   if (!type)
     return failure();
   result.addAttribute(FunctionOpInterface::getTypeAttrName(),

mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp

 //===----------------------------------------------------------------------===//
 
 ParseResult spirv::FuncOp::parse(OpAsmParser &parser, OperationState &state) {
   SmallVector<OpAsmParser::UnresolvedOperand> entryArgs;
   SmallVector<NamedAttrList> argAttrs;
   SmallVector<NamedAttrList> resultAttrs;
   SmallVector<Type> argTypes;
   SmallVector<Type> resultTypes;
-  SmallVector<Location> argLocations;
   auto &builder = parser.getBuilder();
 
   // Parse the name as a symbol.
   StringAttr nameAttr;
   if (parser.parseSymbolName(nameAttr, SymbolTable::getSymbolAttrName(),
                              state.attributes))
     return failure();
 
   // Parse the function signature.
   bool isVariadic = false;
   if (function_interface_impl::parseFunctionSignature(
           parser, /*allowVariadic=*/false, entryArgs, argTypes, argAttrs,
-          argLocations, isVariadic, resultTypes, resultAttrs))
+          isVariadic, resultTypes, resultAttrs))
     return failure();
 
   auto fnType = builder.getFunctionType(argTypes, resultTypes);
   state.addAttribute(FunctionOpInterface::getTypeAttrName(),
                      TypeAttr::get(fnType));
 
   // Parse the optional function control keyword.
   spirv::FunctionControl fnControl;

mlir/lib/IR/FunctionImplementation.cpp

 #include "mlir/IR/SymbolTable.h"
 
 using namespace mlir;
 
 ParseResult mlir::function_interface_impl::parseFunctionArgumentList(
     OpAsmParser &parser, bool allowAttributes, bool allowVariadic,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &argNames,
     SmallVectorImpl<Type> &argTypes, SmallVectorImpl<NamedAttrList> &argAttrs,
-    SmallVectorImpl<Location> &argLocations, bool &isVariadic) {
+    bool &isVariadic) {
   if (parser.parseLParen())
     return failure();
 
   // The argument list either has to consistently have ssa-id's followed by
   // 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()) {
       // 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");
-      argNames.push_back(argument);
 
+      // Parse required type.
       if (parser.parseColonType(argumentType))
         return failure();
     } else if (allowVariadic && succeeded(parser.parseOptionalEllipsis())) {
       isVariadic = true;
       return success();
     } else if (!argNames.empty()) {
       // Reject this if the preceding argument had a name.
       return parser.emitError(loc, "expected SSA identifier");
     } else if (parser.parseType(argumentType)) {
       return failure();
     }
 
     // Add the argument type.
     argTypes.push_back(argumentType);
 
-    // Parse any argument attributes.
+    // Parse any argument attributes and source location information.
     NamedAttrList attrs;
-    if (parser.parseOptionalAttrDict(attrs))
+    if (parser.parseOptionalAttrDict(attrs) ||
+        parser.parseOptionalLocationSpecifier(argument.sourceLoc))
       return failure();
+         
     if (!allowAttributes && !attrs.empty())
       return parser.emitError(loc, "expected arguments without attributes");
     argAttrs.push_back(attrs);
 
-    // Parse a location if specified.
-    Optional<Location> explicitLoc;
-    if (!argument.name.empty() &&
-        parser.parseOptionalLocationSpecifier(explicitLoc))
-      return failure();
-    if (!explicitLoc)
-      explicitLoc = parser.getEncodedSourceLoc(loc);
-    argLocations.push_back(*explicitLoc);
-
+    // If we had an argument name, then remember the parsed argument.
+    if (!argument.name.empty())
+      argNames.push_back(argument);
     return success();
   };
 
   // Parse the function arguments.
   isVariadic = false;
   if (failed(parser.parseOptionalRParen())) {
     do {
       unsigned numTypedArguments = argTypes.size();
parseFunctionResultList(OpAsmParser &parser, SmallVectorImpl<Type> &resultTypes,
   } while (succeeded(parser.parseOptionalComma()));
   return parser.parseRParen();
 }
 
 ParseResult mlir::function_interface_impl::parseFunctionSignature(
     OpAsmParser &parser, bool allowVariadic,
     SmallVectorImpl<OpAsmParser::UnresolvedOperand> &argNames,
     SmallVectorImpl<Type> &argTypes, SmallVectorImpl<NamedAttrList> &argAttrs,
-    SmallVectorImpl<Location> &argLocations, bool &isVariadic,
-    SmallVectorImpl<Type> &resultTypes,
+    bool &isVariadic, SmallVectorImpl<Type> &resultTypes,
     SmallVectorImpl<NamedAttrList> &resultAttrs) {
   bool allowArgAttrs = true;
   if (parseFunctionArgumentList(parser, allowArgAttrs, allowVariadic, argNames,
-                                argTypes, argAttrs, argLocations, isVariadic))
+                                argTypes, argAttrs, isVariadic))
     return failure();
   if (succeeded(parser.parseOptionalArrow()))
     return parseFunctionResultList(parser, resultTypes, resultAttrs);
   return success();
 }
 
 /// Implementation of `addArgAndResultAttrs` that is attribute list type
 /// agnostic.

 ParseResult mlir::function_interface_impl::parseFunctionOp(
     OpAsmParser &parser, OperationState &result, bool allowVariadic,
     FuncTypeBuilder funcTypeBuilder) {
   SmallVector<OpAsmParser::UnresolvedOperand> entryArgs;
   SmallVector<NamedAttrList> argAttrs;
   SmallVector<NamedAttrList> resultAttrs;
   SmallVector<Type> argTypes;
   SmallVector<Type> resultTypes;
-  SmallVector<Location> argLocations;
   auto &builder = parser.getBuilder();
 
   // Parse visibility.
   impl::parseOptionalVisibilityKeyword(parser, result.attributes);
 
   // Parse the name as a symbol.
   StringAttr nameAttr;
   if (parser.parseSymbolName(nameAttr, SymbolTable::getSymbolAttrName(),
                              result.attributes))
     return failure();
 
   // Parse the function signature.
   SMLoc signatureLocation = parser.getCurrentLocation();
   bool isVariadic = false;
   if (parseFunctionSignature(parser, allowVariadic, entryArgs, argTypes,
-                             argAttrs, argLocations, isVariadic, resultTypes,
-                             resultAttrs))
+                             argAttrs, isVariadic, resultTypes, resultAttrs))
     return failure();
 
   std::string errorMessage;
   Type type = funcTypeBuilder(builder, argTypes, resultTypes,
                               VariadicFlag(isVariadic), errorMessage);
   if (!type) {
     return parser.emitError(signatureLocation)
            << "failed to construct function type"
ParseResult mlir::function_interface_impl::parseFunctionOp(
   addArgAndResultAttrs(builder, result, argAttrs, resultAttrs);
 
   // Parse the optional function body. The printer will not print the body if
   // its empty, so disallow parsing of empty body in the parser.
   auto *body = result.addRegion();
   SMLoc loc = parser.getCurrentLocation();
   OptionalParseResult parseResult = parser.parseOptionalRegion(
       *body, entryArgs, entryArgs.empty() ? ArrayRef<Type>() : argTypes,
-      entryArgs.empty() ? ArrayRef<Location>() : argLocations,
       /*enableNameShadowing=*/false);
   if (parseResult.hasValue()) {
     if (failed(*parseResult))
       return failure();
     // Function body was parsed, make sure its not empty.
     if (body->empty())
       return parser.emitError(loc, "expected non-empty function body");
   }

mlir/lib/Parser/Parser.cpp

public:
   /// an object of type 'OperationName'. Otherwise, failure is returned.
   FailureOr<OperationName> parseCustomOperationName();
 
   //===--------------------------------------------------------------------===//
   // Region Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parse a region into 'region' with the provided entry block arguments.
-  /// If non-empty, 'argLocations' contains an optional locations for each
-  /// argument. 'isIsolatedNameScope' indicates if the naming scope of this
-  /// region is isolated from those above.
+  /// 'isIsolatedNameScope' indicates if the naming scope of this region is
+  /// isolated from those above.
   ParseResult
   parseRegion(Region &region,
               ArrayRef<std::pair<UnresolvedOperand, Type>> entryArguments,
-              ArrayRef<Location> argLocations,
               bool isIsolatedNameScope = false);
 
   /// Parse a region body into 'region'.
   ParseResult
   parseRegionBody(Region &region, SMLoc startLoc,
                   ArrayRef<std::pair<UnresolvedOperand, Type>> entryArguments,
-                  ArrayRef<Location> argLocations, bool isIsolatedNameScope);
+                  bool isIsolatedNameScope);
 
   //===--------------------------------------------------------------------===//
   // Block Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parse a new block into 'block'.
   ParseResult parseBlock(Block *&block);
 
if (state.asmState) {
           op, nameTok.getLocRange(), /*endLoc=*/getToken().getLoc(),
           asmResultGroups);
     }
 
     // Add definitions for each of the result groups.
     unsigned opResI = 0;
     for (ResultRecord &resIt : resultIDs) {
       for (unsigned subRes : llvm::seq<unsigned>(0, std::get<1>(resIt))) {
-        if (addDefinition({std::get<2>(resIt), std::get<0>(resIt), subRes},
+        if (addDefinition({std::get<2>(resIt), std::get<0>(resIt), subRes, {}},
                           op->getResult(opResI++)))
           return failure();
       }
     }
 
     // Add this operation to the assembly state if it was provided to populate.
   } else if (state.asmState) {
     state.asmState->finalizeOperationDefinition(op, nameTok.getLocRange(),
ParseResult OperationParser::parseGenericOperationAfterOpName(
   }
 
   // Parse the region list, if not explicitly provided.
   if (!parsedRegions) {
     if (consumeIf(Token::l_paren)) {
       do {
         // Create temporary regions with the top level region as parent.
         result.regions.emplace_back(new Region(topLevelOp));
-        if (parseRegion(*result.regions.back(), /*entryArguments=*/{},
-                        /*argLocations=*/{}))
+        if (parseRegion(*result.regions.back(), /*entryArguments=*/{}))
           return failure();
       } while (consumeIf(Token::comma));
       if (parseToken(Token::r_paren, "expected ')' to end region list"))
         return failure();
     }
   } else {
     result.addRegions(*parsedRegions);
   }
public:
   //===--------------------------------------------------------------------===//
 
   /// Parse a single operand.
   ParseResult parseOperand(UnresolvedOperand &result) override {
     OperationParser::UnresolvedOperand useInfo;
     if (parser.parseSSAUse(useInfo))
       return failure();
 
-    result = {useInfo.location, useInfo.name, useInfo.number};
-    return success();
+    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 {
     if (parser.getToken().is(Token::percent_identifier))
       return parseOperand(result);
     return llvm::None;
   }
 
public:
   //===--------------------------------------------------------------------===//
   // Region Parsing
   //===--------------------------------------------------------------------===//
 
   /// Parse a region that takes `arguments` of `argTypes` types.  This
   /// effectively defines the SSA values of `arguments` and assigns their type.
   ParseResult parseRegion(Region &region, ArrayRef<UnresolvedOperand> arguments,
                           ArrayRef<Type> argTypes,
-                          ArrayRef<Location> argLocations,
                           bool enableNameShadowing) override {
     assert(arguments.size() == argTypes.size() &&
            "mismatching number of arguments and types");
 
     SmallVector<std::pair<OperationParser::UnresolvedOperand, Type>, 2>
         regionArguments;
     for (auto pair : llvm::zip(arguments, argTypes))
       regionArguments.emplace_back(std::get<0>(pair), std::get<1>(pair));
 
     // Try to parse the region.
     (void)isIsolatedFromAbove;
     assert((!enableNameShadowing || isIsolatedFromAbove) &&
            "name shadowing is only allowed on isolated regions");
-    if (parser.parseRegion(region, regionArguments, argLocations,
-                           enableNameShadowing))
+    if (parser.parseRegion(region, regionArguments, enableNameShadowing))
       return failure();
     return success();
   }
 
   /// Parses a region if present.
   OptionalParseResult parseOptionalRegion(Region &region,
                                           ArrayRef<UnresolvedOperand> arguments,
                                           ArrayRef<Type> argTypes,
-                                          ArrayRef<Location> argLocations,
                                           bool enableNameShadowing) override {
     if (parser.getToken().isNot(Token::l_brace))
       return llvm::None;
-    return parseRegion(region, arguments, argTypes, argLocations,
-                       enableNameShadowing);
+    return parseRegion(region, arguments, argTypes, enableNameShadowing);
   }
 
   /// Parses a region if present. If the region is present, a new region is
   /// allocated and placed in `region`. If no region is present, `region`
   /// remains untouched.
   OptionalParseResult parseOptionalRegion(
       std::unique_ptr<Region> &region, ArrayRef<UnresolvedOperand> arguments,
       ArrayRef<Type> argTypes, bool enableNameShadowing = false) override {
     if (parser.getToken().isNot(Token::l_brace))
       return llvm::None;
     std::unique_ptr<Region> newRegion = std::make_unique<Region>();
-    if (parseRegion(*newRegion, arguments, argTypes, /*argLocations=*/{},
-                    enableNameShadowing))
+    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`.

 //===----------------------------------------------------------------------===//
 // Region Parsing
 //===----------------------------------------------------------------------===//
 
 ParseResult OperationParser::parseRegion(
     Region &region,
     ArrayRef<std::pair<OperationParser::UnresolvedOperand, Type>>
         entryArguments,
-    ArrayRef<Location> argLocations, bool isIsolatedNameScope) {
+    bool isIsolatedNameScope) {
   // Parse the '{'.
   Token lBraceTok = getToken();
   if (parseToken(Token::l_brace, "expected '{' to begin a region"))
     return failure();
 
   // If we are populating the parser state, start a new region definition.
   if (state.asmState)
     state.asmState->startRegionDefinition();
 
   // Parse the region body.
   if ((!entryArguments.empty() || getToken().isNot(Token::r_brace)) &&
-      parseRegionBody(region, lBraceTok.getLoc(), entryArguments, argLocations,
+      parseRegionBody(region, lBraceTok.getLoc(), entryArguments,
                       isIsolatedNameScope)) {
     return failure();
   }
   consumeToken(Token::r_brace);
 
   // If we are populating the parser state, finalize this region.
   if (state.asmState)
     state.asmState->finalizeRegionDefinition();
 
   return success();
 }
 
 ParseResult OperationParser::parseRegionBody(
     Region &region, SMLoc startLoc,
     ArrayRef<std::pair<OperationParser::UnresolvedOperand, Type>>
         entryArguments,
-    ArrayRef<Location> argLocations, bool isIsolatedNameScope) {
-  assert(argLocations.empty() || argLocations.size() == entryArguments.size());
+    bool isIsolatedNameScope) {
   auto currentPt = opBuilder.saveInsertionPoint();
 
   // Push a new named value scope.
   pushSSANameScope(isIsolatedNameScope);
 
   // Parse the first block directly to allow for it to be unnamed.
   auto owningBlock = std::make_unique<Block>();
   Block *block = owningBlock.get();
 
   // If this block is not defined in the source file, add a definition for it
   // now in the assembly state. Blocks with a name will be defined when the name
   // is parsed.
   if (state.asmState && getToken().isNot(Token::caret_identifier))
     state.asmState->addDefinition(block, startLoc);
 
   // Add arguments to the entry block.
   if (!entryArguments.empty()) {
     // If we had named arguments, then don't allow a block name.
     if (getToken().is(Token::caret_identifier))
       return emitError("invalid block name in region with named arguments");
 
-    for (const auto &it : llvm::enumerate(entryArguments)) {
-      size_t argIndex = it.index();
-      auto &placeholderArgPair = it.value();
+    for (auto &placeholderArgPair : entryArguments) {
       auto &argInfo = placeholderArgPair.first;
 
       // Ensure that the argument was not already defined.
       if (auto defLoc = getReferenceLoc(argInfo.name, argInfo.number)) {
         return emitError(argInfo.location, "region entry argument '" +
                                                argInfo.name +
                                                "' is already in use")
                    .attachNote(getEncodedSourceLocation(*defLoc))
                << "previously referenced here";
       }
-      BlockArgument arg = block->addArgument(
-          placeholderArgPair.second,
-          argLocations.empty()
-              ? getEncodedSourceLocation(placeholderArgPair.first.location)
-              : argLocations[argIndex]);
+      Location loc = argInfo.sourceLoc.hasValue()
+                         ? argInfo.sourceLoc.getValue()
+                         : getEncodedSourceLocation(argInfo.location);
+      BlockArgument arg = block->addArgument(placeholderArgPair.second, loc);
 
       // Add a definition of this arg to the assembly state if provided.
       if (state.asmState)
         state.asmState->addDefinition(arg, argInfo.location);
 
       // Record the definition for this argument.
       if (addDefinition(argInfo, arg))
         return failure();

mlir/test/Dialect/LLVMIR/func.mlir

 // RUN: mlir-opt -split-input-file -verify-diagnostics %s | mlir-opt | FileCheck %s
 // RUN: mlir-opt -split-input-file -verify-diagnostics -mlir-print-op-generic %s | FileCheck %s --check-prefix=GENERIC
+// RUN: mlir-opt -split-input-file -verify-diagnostics %s -mlir-print-debuginfo | mlir-opt -mlir-print-debuginfo | FileCheck %s --check-prefix=LOCINFO
 
 module {
   // GENERIC: "llvm.func"
   // GENERIC: function_type = !llvm.func<void ()>
   // GENERIC-SAME: sym_name = "foo"
   // GENERIC-SAME: () -> ()
   // CHECK: llvm.func @foo()
   "llvm.func"() ({
module {
   }
 
   // CHECK: llvm.func @byvalattr(%{{.*}}: !llvm.ptr<i32> {llvm.byval})
   llvm.func @byvalattr(%arg0: !llvm.ptr<i32> {llvm.byval}) {
     llvm.return
   }
 
   // CHECK: llvm.func @sretattr(%{{.*}}: !llvm.ptr<i32> {llvm.sret})
-  llvm.func @sretattr(%arg0: !llvm.ptr<i32> {llvm.sret}) {
+  // LOCINFO: llvm.func @sretattr(%{{.*}}: !llvm.ptr<i32> {llvm.sret} loc("some_source_loc"))
+  llvm.func @sretattr(%arg0: !llvm.ptr<i32> {llvm.sret} loc("some_source_loc")) {
     llvm.return
   }
 
   // CHECK: llvm.func @nestattr(%{{.*}}: !llvm.ptr<i32> {llvm.nest})
   llvm.func @nestattr(%arg0: !llvm.ptr<i32> {llvm.nest}) {
     llvm.return
   }
 

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

ParseResult IsolatedRegionOp::parse(OpAsmParser &parser,
 
   // Parse the input operand.
   if (parser.parseOperand(argInfo) ||
       parser.resolveOperand(argInfo, argType, result.operands))
     return failure();
 
   // Parse the body region, and reuse the operand info as the argument info.
   Region *body = result.addRegion();
-  return parser.parseRegion(*body, argInfo, argType, /*argLocations=*/{},
+  return parser.parseRegion(*body, argInfo, argType,
                             /*enableNameShadowing=*/true);
 }
 
 void IsolatedRegionOp::print(OpAsmPrinter &p) {
   p << "test.isolated_region ";
   p.printOperand(getOperand());
   p.shadowRegionArgs(getRegion(), getOperand());
   p << ' ';