[Mlir-commits] [mlir] 80d981e - Revert "[IR] Add a Location to BlockArgument." and follow-on commit

Richard Smith llvmlistbot at llvm.org
Tue May 18 19:26:17 PDT 2021


Author: Richard Smith
Date: 2021-05-18T19:26:00-07:00
New Revision: 80d981eda69f1ada6d944ed89571456cad13b850

URL: https://github.com/llvm/llvm-project/commit/80d981eda69f1ada6d944ed89571456cad13b850
DIFF: https://github.com/llvm/llvm-project/commit/80d981eda69f1ada6d944ed89571456cad13b850.diff

LOG: Revert "[IR] Add a Location to BlockArgument." and follow-on commit
"[mlir] Speed up Lexer::getEncodedSourceLocation"

This reverts commit 3043be9d2db4d0cdf079adb5e1bdff032405e941 and commit
861d69a5259653f60d59795597493a7939b794fe.

This change resulted in printing textual MLIR that can't be parsed; see
review thread https://reviews.llvm.org/D102567 for details.

Added: 
    

Modified: 
    mlir/include/mlir/IR/Block.h
    mlir/include/mlir/IR/Builders.h
    mlir/include/mlir/IR/OpImplementation.h
    mlir/include/mlir/IR/Value.h
    mlir/lib/IR/AsmPrinter.cpp
    mlir/lib/IR/Block.cpp
    mlir/lib/IR/Builders.cpp
    mlir/lib/IR/FunctionImplementation.cpp
    mlir/lib/IR/Value.cpp
    mlir/lib/Parser/Lexer.cpp
    mlir/lib/Parser/Parser.cpp
    mlir/test/IR/locations.mlir
    mlir/test/Transforms/test-legalize-type-conversion.mlir
    mlir/test/mlir-tblgen/pattern.mlir

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/IR/Block.h b/mlir/include/mlir/IR/Block.h
index cafcf585bf2d..f5436ef49d3c 100644
--- a/mlir/include/mlir/IR/Block.h
+++ b/mlir/include/mlir/IR/Block.h
@@ -22,8 +22,7 @@ class BitVector;
 
 namespace mlir {
 class TypeRange;
-template <typename ValueRangeT>
-class ValueTypeRange;
+template <typename ValueRangeT> class ValueTypeRange;
 
 /// `Block` represents an ordered list of `Operation`s.
 class Block : public IRObjectWithUseList<BlockOperand>,
@@ -88,21 +87,18 @@ class Block : public IRObjectWithUseList<BlockOperand>,
   bool args_empty() { return arguments.empty(); }
 
   /// Add one value to the argument list.
-  BlockArgument addArgument(Type type, Optional<Location> loc = {});
+  BlockArgument addArgument(Type type);
 
   /// Insert one value to the position in the argument list indicated by the
   /// given iterator. The existing arguments are shifted. The block is expected
   /// not to have predecessors.
-  BlockArgument insertArgument(args_iterator it, Type type,
-                               Optional<Location> loc = {});
+  BlockArgument insertArgument(args_iterator it, Type type);
 
   /// Add one argument to the argument list for each type specified in the list.
-  iterator_range<args_iterator> addArguments(TypeRange types,
-                                             ArrayRef<Location> locs = {});
+  iterator_range<args_iterator> addArguments(TypeRange types);
 
   /// Add one value to the argument list at the specified position.
-  BlockArgument insertArgument(unsigned index, Type type,
-                               Optional<Location> loc = {});
+  BlockArgument insertArgument(unsigned index, Type type);
 
   /// Erase the argument at 'index' and remove it from the argument list.
   void eraseArgument(unsigned index);
@@ -181,18 +177,15 @@ class Block : public IRObjectWithUseList<BlockOperand>,
 
   /// Return an iterator range over the operations within this block that are of
   /// 'OpT'.
-  template <typename OpT>
-  iterator_range<op_iterator<OpT>> getOps() {
+  template <typename OpT> iterator_range<op_iterator<OpT>> getOps() {
     auto endIt = end();
     return {detail::op_filter_iterator<OpT, iterator>(begin(), endIt),
             detail::op_filter_iterator<OpT, iterator>(endIt, endIt)};
   }
-  template <typename OpT>
-  op_iterator<OpT> op_begin() {
+  template <typename OpT> op_iterator<OpT> op_begin() {
     return detail::op_filter_iterator<OpT, iterator>(begin(), end());
   }
-  template <typename OpT>
-  op_iterator<OpT> op_end() {
+  template <typename OpT> op_iterator<OpT> op_end() {
     return detail::op_filter_iterator<OpT, iterator>(end(), end());
   }
 

diff  --git a/mlir/include/mlir/IR/Builders.h b/mlir/include/mlir/IR/Builders.h
index 1788fa715c2f..1e0863c7a7a4 100644
--- a/mlir/include/mlir/IR/Builders.h
+++ b/mlir/include/mlir/IR/Builders.h
@@ -79,8 +79,7 @@ class Builder {
   NoneType getNoneType();
 
   /// Get or construct an instance of the type 'ty' with provided arguments.
-  template <typename Ty, typename... Args>
-  Ty getType(Args... args) {
+  template <typename Ty, typename... Args> Ty getType(Args... args) {
     return Ty::get(context, args...);
   }
 
@@ -373,13 +372,11 @@ class OpBuilder : public Builder {
   /// end of it. The block is inserted at the provided insertion point of
   /// 'parent'.
   Block *createBlock(Region *parent, Region::iterator insertPt = {},
-                     TypeRange argTypes = llvm::None,
-                     ArrayRef<Location> locs = {});
+                     TypeRange argTypes = llvm::None);
 
   /// Add new block with 'argTypes' arguments and set the insertion point to the
   /// end of it. The block is placed before 'insertBefore'.
-  Block *createBlock(Block *insertBefore, TypeRange argTypes = llvm::None,
-                     ArrayRef<Location> locs = {});
+  Block *createBlock(Block *insertBefore, TypeRange argTypes = llvm::None);
 
   //===--------------------------------------------------------------------===//
   // Operation Creation
@@ -475,8 +472,7 @@ class OpBuilder : public Builder {
   Operation *cloneWithoutRegions(Operation &op) {
     return insert(op.cloneWithoutRegions());
   }
-  template <typename OpT>
-  OpT cloneWithoutRegions(OpT op) {
+  template <typename OpT> OpT cloneWithoutRegions(OpT op) {
     return cast<OpT>(cloneWithoutRegions(*op.getOperation()));
   }
 

diff  --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index a9c96473a3a1..db1e98d95b96 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -40,15 +40,6 @@ class OpAsmPrinter {
   /// operation.
   virtual void printNewline() = 0;
 
-  /// Print a block argument in the usual format of:
-  ///   %ssaName : type {attr1=42} loc("here")
-  /// where location printing is controlled by the standard internal option.
-  /// You may pass omitType=true to not print a type, and pass an empty
-  /// attribute list if you don't care for attributes.
-  virtual void printRegionArgument(BlockArgument arg,
-                                   ArrayRef<NamedAttribute> argAttrs = {},
-                                   bool omitType = false) = 0;
-
   /// Print implementations for various things an operation contains.
   virtual void printOperand(Value value) = 0;
   virtual void printOperand(Value value, raw_ostream &os) = 0;
@@ -587,10 +578,6 @@ class OpAsmParser {
                                               StringRef attrName,
                                               NamedAttrList &attrs) = 0;
 
-  /// Parse a loc(...) specifier if present, filling in result if so.
-  virtual ParseResult
-  parseOptionalLocationSpecifier(Optional<Location> &result) = 0;
-
   //===--------------------------------------------------------------------===//
   // Operand Parsing
   //===--------------------------------------------------------------------===//

diff  --git a/mlir/include/mlir/IR/Value.h b/mlir/include/mlir/IR/Value.h
index 1901802e3dff..bd80b2b582d1 100644
--- a/mlir/include/mlir/IR/Value.h
+++ b/mlir/include/mlir/IR/Value.h
@@ -85,8 +85,7 @@ class Value {
   Value(const Value &) = default;
   Value &operator=(const Value &) = default;
 
-  template <typename U>
-  bool isa() const {
+  template <typename U> bool isa() const {
     assert(*this && "isa<> used on a null type.");
     return U::classof(*this);
   }
@@ -95,16 +94,13 @@ class Value {
   bool isa() const {
     return isa<First>() || isa<Second, Rest...>();
   }
-  template <typename U>
-  U dyn_cast() const {
+  template <typename U> U dyn_cast() const {
     return isa<U>() ? U(impl) : U(nullptr);
   }
-  template <typename U>
-  U dyn_cast_or_null() const {
+  template <typename U> U dyn_cast_or_null() const {
     return (*this && isa<U>()) ? U(impl) : U(nullptr);
   }
-  template <typename U>
-  U cast() const {
+  template <typename U> U cast() const {
     assert(isa<U>());
     return U(impl);
   }
@@ -138,9 +134,9 @@ class Value {
     return llvm::dyn_cast_or_null<OpTy>(getDefiningOp());
   }
 
-  /// Return the location of this value.
+  /// If this value is the result of an operation, use it as a location,
+  /// otherwise return an unknown location.
   Location getLoc() const;
-  void setLoc(Location loc);
 
   /// Return the Region in which this Value is defined.
   Region *getParentRegion();
@@ -254,9 +250,8 @@ class BlockArgumentImpl : public ValueImpl {
   }
 
 private:
-  BlockArgumentImpl(Type type, Block *owner, int64_t index, Location loc)
-      : ValueImpl(type, Kind::BlockArgument), owner(owner), index(index),
-        loc(loc) {}
+  BlockArgumentImpl(Type type, Block *owner, int64_t index)
+      : ValueImpl(type, Kind::BlockArgument), owner(owner), index(index) {}
 
   /// The owner of this argument.
   Block *owner;
@@ -264,9 +259,6 @@ class BlockArgumentImpl : public ValueImpl {
   /// The position in the argument list.
   int64_t index;
 
-  /// The source location of this argument.
-  Location loc;
-
   /// Allow access to owner and constructor.
   friend BlockArgument;
 };
@@ -287,15 +279,10 @@ class BlockArgument : public Value {
   /// Returns the number of this argument.
   unsigned getArgNumber() const { return getImpl()->index; }
 
-  /// Return the location for this argument.
-  Location getLoc() const { return getImpl()->loc; }
-  void setLoc(Location loc) { getImpl()->loc = loc; }
-
 private:
   /// Allocate a new argument with the given type and owner.
-  static BlockArgument create(Type type, Block *owner, int64_t index,
-                              Location loc) {
-    return new detail::BlockArgumentImpl(type, owner, index, loc);
+  static BlockArgument create(Type type, Block *owner, int64_t index) {
+    return new detail::BlockArgumentImpl(type, owner, index);
   }
 
   /// Destroy and deallocate this argument.
@@ -439,8 +426,7 @@ inline ::llvm::hash_code hash_value(Value arg) {
 
 namespace llvm {
 
-template <>
-struct DenseMapInfo<mlir::Value> {
+template <> struct DenseMapInfo<mlir::Value> {
   static mlir::Value getEmptyKey() {
     void *pointer = llvm::DenseMapInfo<void *>::getEmptyKey();
     return mlir::Value::getFromOpaquePointer(pointer);
@@ -467,8 +453,7 @@ struct DenseMapInfo<mlir::BlockArgument> : public DenseMapInfo<mlir::Value> {
 };
 
 /// Allow stealing the low bits of a value.
-template <>
-struct PointerLikeTypeTraits<mlir::Value> {
+template <> struct PointerLikeTypeTraits<mlir::Value> {
 public:
   static inline void *getAsVoidPointer(mlir::Value value) {
     return const_cast<void *>(value.getAsOpaquePointer());

diff  --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index df803c889ccf..d36f5839d7c3 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -485,8 +485,6 @@ class DummyAliasOperationPrinter : private OpAsmPrinter {
   void printSuccessor(Block *) override {}
   void printSuccessorAndUseList(Block *, ValueRange) override {}
   void shadowRegionArgs(Region &, ValueRange) override {}
-  void printRegionArgument(BlockArgument arg, ArrayRef<NamedAttribute> argAttrs,
-                           bool omitType) override {}
 
   /// The printer flags to use when determining potential aliases.
   const OpPrintingFlags &printerFlags;
@@ -2347,15 +2345,6 @@ class OperationPrinter : public ModulePrinter, private OpAsmPrinter {
     ModulePrinter::printAttribute(attr, AttrTypeElision::Must);
   }
 
-  /// Print a block argument in the usual format of:
-  ///   %ssaName : type {attr1=42} loc("here")
-  /// where location printing is controlled by the standard internal option.
-  /// You may pass omitType=true to not print a type, and pass an empty
-  /// attribute list if you don't care for attributes.
-  void printRegionArgument(BlockArgument arg,
-                           ArrayRef<NamedAttribute> argAttrs = {},
-                           bool omitType = false) override;
-
   /// Print the ID for the given value.
   void printOperand(Value value) override { printValueID(value); }
   void printOperand(Value value, raw_ostream &os) override {
@@ -2430,23 +2419,6 @@ void OperationPrinter::printTopLevelOperation(Operation *op) {
   state->getAliasState().printDeferredAliases(os, newLine);
 }
 
-/// Print a block argument in the usual format of:
-///   %ssaName : type {attr1=42} loc("here")
-/// where location printing is controlled by the standard internal option.
-/// You may pass omitType=true to not print a type, and pass an empty
-/// attribute list if you don't care for attributes.
-void OperationPrinter::printRegionArgument(BlockArgument arg,
-                                           ArrayRef<NamedAttribute> argAttrs,
-                                           bool omitType) {
-  printOperand(arg);
-  if (!omitType) {
-    os << ": ";
-    printType(arg.getType());
-  }
-  printOptionalAttrDict(argAttrs);
-  printTrailingLocation(arg.getLoc());
-}
-
 void OperationPrinter::print(Operation *op) {
   // Track the location of this operation.
   state->registerOperationLocation(op, newLine.curLine, currentIndent);
@@ -2557,7 +2529,6 @@ void OperationPrinter::print(Block *block, bool printBlockArgs,
         printValueID(arg);
         os << ": ";
         printType(arg.getType());
-        printTrailingLocation(arg.getLoc());
       });
       os << ')';
     }
@@ -2729,7 +2700,7 @@ void IntegerSet::print(raw_ostream &os) const {
 void Value::print(raw_ostream &os) {
   if (auto *op = getDefiningOp())
     return op->print(os);
-  // TODO: Improve BlockArgument print'ing.
+  // TODO: Improve this.
   BlockArgument arg = this->cast<BlockArgument>();
   os << "<block argument> of type '" << arg.getType()
      << "' at index: " << arg.getArgNumber() << '\n';
@@ -2738,7 +2709,7 @@ void Value::print(raw_ostream &os, AsmState &state) {
   if (auto *op = getDefiningOp())
     return op->print(os, state);
 
-  // TODO: Improve BlockArgument print'ing.
+  // TODO: Improve this.
   BlockArgument arg = this->cast<BlockArgument>();
   os << "<block argument> of type '" << arg.getType()
      << "' at index: " << arg.getArgNumber() << '\n';

diff  --git a/mlir/lib/IR/Block.cpp b/mlir/lib/IR/Block.cpp
index d05378806776..d390d490b176 100644
--- a/mlir/lib/IR/Block.cpp
+++ b/mlir/lib/IR/Block.cpp
@@ -138,54 +138,23 @@ auto Block::getArgumentTypes() -> ValueTypeRange<BlockArgListType> {
   return ValueTypeRange<BlockArgListType>(getArguments());
 }
 
-BlockArgument Block::addArgument(Type type, Optional<Location> loc) {
-  // TODO: Require locations for BlockArguments.
-  if (!loc.hasValue()) {
-    // Use the location of the parent operation if the block is attached.
-    if (Operation *parentOp = getParentOp())
-      loc = parentOp->getLoc();
-    else
-      loc = UnknownLoc::get(type.getContext());
-  }
-
-  BlockArgument arg = BlockArgument::create(type, this, arguments.size(), *loc);
+BlockArgument Block::addArgument(Type type) {
+  BlockArgument arg = BlockArgument::create(type, this, arguments.size());
   arguments.push_back(arg);
   return arg;
 }
 
 /// Add one argument to the argument list for each type specified in the list.
-auto Block::addArguments(TypeRange types, ArrayRef<Location> locs)
-    -> iterator_range<args_iterator> {
-  // TODO: Require locations for BlockArguments.
-  assert((locs.empty() || types.size() == locs.size()) &&
-         "incorrect number of block argument locations");
+auto Block::addArguments(TypeRange types) -> iterator_range<args_iterator> {
   size_t initialSize = arguments.size();
-
   arguments.reserve(initialSize + types.size());
-
-  // TODO: Require locations for BlockArguments.
-  if (locs.empty()) {
-    for (auto type : types)
-      addArgument(type);
-  } else {
-    for (auto typeAndLoc : llvm::zip(types, locs))
-      addArgument(std::get<0>(typeAndLoc), std::get<1>(typeAndLoc));
-  }
+  for (auto type : types)
+    addArgument(type);
   return {arguments.data() + initialSize, arguments.data() + arguments.size()};
 }
 
-BlockArgument Block::insertArgument(unsigned index, Type type,
-                                    Optional<Location> loc) {
-  // TODO: Require locations for BlockArguments.
-  if (!loc.hasValue()) {
-    // Use the location of the parent operation if the block is attached.
-    if (Operation *parentOp = getParentOp())
-      loc = parentOp->getLoc();
-    else
-      loc = UnknownLoc::get(type.getContext());
-  }
-
-  auto arg = BlockArgument::create(type, this, index, *loc);
+BlockArgument Block::insertArgument(unsigned index, Type type) {
+  auto arg = BlockArgument::create(type, this, index);
   assert(index <= arguments.size());
   arguments.insert(arguments.begin() + index, arg);
   // Update the cached position for all the arguments after the newly inserted
@@ -198,11 +167,10 @@ BlockArgument Block::insertArgument(unsigned index, Type type,
 
 /// Insert one value to the given position of the argument list. The existing
 /// arguments are shifted. The block is expected not to have predecessors.
-BlockArgument Block::insertArgument(args_iterator it, Type type,
-                                    Optional<Location> loc) {
+BlockArgument Block::insertArgument(args_iterator it, Type type) {
   assert(llvm::empty(getPredecessors()) &&
          "cannot insert arguments to blocks with predecessors");
-  return insertArgument(it->getArgNumber(), type, loc);
+  return insertArgument(it->getArgNumber(), type);
 }
 
 void Block::eraseArgument(unsigned index) {

diff  --git a/mlir/lib/IR/Builders.cpp b/mlir/lib/IR/Builders.cpp
index 737ec7446199..4f8aa9e82075 100644
--- a/mlir/lib/IR/Builders.cpp
+++ b/mlir/lib/IR/Builders.cpp
@@ -371,13 +371,13 @@ Operation *OpBuilder::insert(Operation *op) {
 /// end of it. The block is inserted at the provided insertion point of
 /// 'parent'.
 Block *OpBuilder::createBlock(Region *parent, Region::iterator insertPt,
-                              TypeRange argTypes, ArrayRef<Location> locs) {
+                              TypeRange argTypes) {
   assert(parent && "expected valid parent region");
   if (insertPt == Region::iterator())
     insertPt = parent->end();
 
   Block *b = new Block();
-  b->addArguments(argTypes, locs);
+  b->addArguments(argTypes);
   parent->getBlocks().insert(insertPt, b);
   setInsertionPointToEnd(b);
 
@@ -388,11 +388,10 @@ Block *OpBuilder::createBlock(Region *parent, Region::iterator insertPt,
 
 /// Add new block with 'argTypes' arguments and set the insertion point to the
 /// end of it.  The block is placed before 'insertBefore'.
-Block *OpBuilder::createBlock(Block *insertBefore, TypeRange argTypes,
-                              ArrayRef<Location> locs) {
+Block *OpBuilder::createBlock(Block *insertBefore, TypeRange argTypes) {
   assert(insertBefore && "expected valid insertion block");
   return createBlock(insertBefore->getParent(), Region::iterator(insertBefore),
-                     argTypes, locs);
+                     argTypes);
 }
 
 /// Create an operation given the fields represented as an OperationState.

diff  --git a/mlir/lib/IR/FunctionImplementation.cpp b/mlir/lib/IR/FunctionImplementation.cpp
index a4ea0400003c..aadf5456126b 100644
--- a/mlir/lib/IR/FunctionImplementation.cpp
+++ b/mlir/lib/IR/FunctionImplementation.cpp
@@ -59,13 +59,6 @@ ParseResult mlir::function_like_impl::parseFunctionArgumentList(
     if (!allowAttributes && !attrs.empty())
       return parser.emitError(loc, "expected arguments without attributes");
     argAttrs.push_back(attrs);
-
-    // Parse a location if specified.  TODO: Don't drop it on the floor.
-    Optional<Location> explicitLoc;
-    if (!argument.name.empty() &&
-        parser.parseOptionalLocationSpecifier(explicitLoc))
-      return failure();
-
     return success();
   };
 
@@ -305,15 +298,13 @@ void mlir::function_like_impl::printFunctionSignature(
       p << ", ";
 
     if (!isExternal) {
-      ArrayRef<NamedAttribute> attrs;
-      if (argAttrs)
-        attrs = argAttrs[i].cast<DictionaryAttr>().getValue();
-      p.printRegionArgument(body.getArgument(i), attrs);
-    } else {
-      p.printType(argTypes[i]);
-      if (argAttrs)
-        p.printOptionalAttrDict(argAttrs[i].cast<DictionaryAttr>().getValue());
+      p.printOperand(body.getArgument(i));
+      p << ": ";
     }
+
+    p.printType(argTypes[i]);
+    if (argAttrs)
+      p.printOptionalAttrDict(argAttrs[i].cast<DictionaryAttr>().getValue());
   }
 
   if (isVariadic) {

diff  --git a/mlir/lib/IR/Value.cpp b/mlir/lib/IR/Value.cpp
index 1ff0bba64fdf..a4baa9311001 100644
--- a/mlir/lib/IR/Value.cpp
+++ b/mlir/lib/IR/Value.cpp
@@ -27,14 +27,10 @@ Location Value::getLoc() const {
   if (auto *op = getDefiningOp())
     return op->getLoc();
 
-  return cast<BlockArgument>().getLoc();
-}
-
-void Value::setLoc(Location loc) {
-  if (auto *op = getDefiningOp())
-    return op->setLoc(loc);
-
-  return cast<BlockArgument>().setLoc(loc);
+  // Use the location of the parent operation if this is a block argument.
+  // TODO: Should we just add locations to block arguments?
+  Operation *parentOp = cast<BlockArgument>().getOwner()->getParentOp();
+  return parentOp ? parentOp->getLoc() : UnknownLoc::get(getContext());
 }
 
 /// Return the Region in which this Value is defined.

diff  --git a/mlir/lib/Parser/Lexer.cpp b/mlir/lib/Parser/Lexer.cpp
index a8ecc7162dc6..763f154687cc 100644
--- a/mlir/lib/Parser/Lexer.cpp
+++ b/mlir/lib/Parser/Lexer.cpp
@@ -41,14 +41,11 @@ Lexer::Lexer(const llvm::SourceMgr &sourceMgr, MLIRContext *context)
 Location Lexer::getEncodedSourceLocation(llvm::SMLoc loc) {
   auto &sourceMgr = getSourceMgr();
   unsigned mainFileID = sourceMgr.getMainFileID();
-  auto &bufferInfo = sourceMgr.getBufferInfo(mainFileID);
-  unsigned lineNo = bufferInfo.getLineNumber(loc.getPointer());
-  unsigned column =
-      (loc.getPointer() - bufferInfo.getPointerForLineNumber(lineNo)) + 1;
+  auto lineAndColumn = sourceMgr.getLineAndColumn(loc, mainFileID);
   auto *buffer = sourceMgr.getMemoryBuffer(mainFileID);
 
-  return FileLineColLoc::get(context, buffer->getBufferIdentifier(), lineNo,
-                             column);
+  return FileLineColLoc::get(context, buffer->getBufferIdentifier(),
+                             lineAndColumn.first, lineAndColumn.second);
 }
 
 /// emitError - Emit an error message and return an Token::error token.

diff  --git a/mlir/lib/Parser/Parser.cpp b/mlir/lib/Parser/Parser.cpp
index f88f94cc57d9..fea26bcb2e32 100644
--- a/mlir/lib/Parser/Parser.cpp
+++ b/mlir/lib/Parser/Parser.cpp
@@ -249,17 +249,11 @@ class OperationParser : public Parser {
   Operation *parseGenericOperation(Block *insertBlock,
                                    Block::iterator insertPt);
 
-  /// This type is used to keep track of things that are either an Operation or
-  /// a BlockArgument.  We cannot use Value for this, because not all Operations
-  /// have results.
-  using OpOrArgument = llvm::PointerUnion<Operation *, BlockArgument>;
-
-  /// Parse an optional trailing location and add it to the specifier Operation
-  /// or `OperandType` if present.
+  /// Parse an optional trailing location for the given operation.
   ///
   ///   trailing-location ::= (`loc` (`(` location `)` | attribute-alias))?
   ///
-  ParseResult parseTrailingLocationSpecifier(OpOrArgument opOrArgument);
+  ParseResult parseTrailingOperationLocation(Operation *op);
 
   /// This is the structure of a result specifier in the assembly syntax,
   /// including the name, number of results, and location.
@@ -391,8 +385,7 @@ class OperationParser : public Parser {
 
   /// A set of operations whose locations reference aliases that have yet to
   /// be resolved.
-  SmallVector<std::pair<OpOrArgument, Token>, 8>
-      opsAndArgumentsWithDeferredLocs;
+  SmallVector<std::pair<Operation *, Token>, 8> opsWithDeferredLocs;
 
   /// The builder used when creating parsed operation instances.
   OpBuilder opBuilder;
@@ -440,7 +433,7 @@ ParseResult OperationParser::finalize() {
 
   // Resolve the locations of any deferred operations.
   auto &attributeAliases = state.symbols.attributeAliasDefinitions;
-  for (std::pair<OpOrArgument, Token> &it : opsAndArgumentsWithDeferredLocs) {
+  for (std::pair<Operation *, Token> &it : opsWithDeferredLocs) {
     llvm::SMLoc tokLoc = it.second.getLoc();
     StringRef identifier = it.second.getSpelling().drop_front();
     Attribute attr = attributeAliases.lookup(identifier);
@@ -451,11 +444,7 @@ ParseResult OperationParser::finalize() {
     if (!locAttr)
       return emitError(tokLoc)
              << "expected location, but found '" << attr << "'";
-    auto opOrArgument = it.first;
-    if (auto *op = opOrArgument.dyn_cast<Operation *>())
-      op->setLoc(locAttr);
-    else
-      opOrArgument.get<BlockArgument>().setLoc(locAttr);
+    it.first->setLoc(locAttr);
   }
 
   // Pop the top level name scope.
@@ -974,7 +963,7 @@ Operation *OperationParser::parseGenericOperation() {
 
   // Create the operation and try to parse a location for it.
   Operation *op = opBuilder.createOperation(result);
-  if (parseTrailingLocationSpecifier(op))
+  if (parseTrailingOperationLocation(op))
     return nullptr;
   return op;
 }
@@ -1370,22 +1359,6 @@ class CustomOpAsmParser : public OpAsmParser {
     return success();
   }
 
-  /// Parse a loc(...) specifier if present, filling in result if so.
-  ParseResult
-  parseOptionalLocationSpecifier(Optional<Location> &result) override {
-    // If there is a 'loc' we parse a trailing location.
-    if (!parser.consumeIf(Token::kw_loc))
-      return success();
-    LocationAttr directLoc;
-    if (parser.parseToken(Token::l_paren, "expected '(' in location") ||
-        parser.parseLocationInstance(directLoc) ||
-        parser.parseToken(Token::r_paren, "expected ')' in location"))
-      return failure();
-
-    result = directLoc;
-    return success();
-  }
-
   //===--------------------------------------------------------------------===//
   // Operand Parsing
   //===--------------------------------------------------------------------===//
@@ -1873,13 +1846,12 @@ OperationParser::parseCustomOperation(ArrayRef<ResultRecord> resultIDs) {
 
   // Otherwise, create the operation and try to parse a location for it.
   Operation *op = opBuilder.createOperation(opState);
-  if (parseTrailingLocationSpecifier(op))
+  if (parseTrailingOperationLocation(op))
     return nullptr;
   return op;
 }
 
-ParseResult
-OperationParser::parseTrailingLocationSpecifier(OpOrArgument opOrArgument) {
+ParseResult OperationParser::parseTrailingOperationLocation(Operation *op) {
   // If there is a 'loc' we parse a trailing location.
   if (!consumeIf(Token::kw_loc))
     return success();
@@ -1907,7 +1879,7 @@ OperationParser::parseTrailingLocationSpecifier(OpOrArgument opOrArgument) {
                << "expected location, but found '" << attr << "'";
     } else {
       // Otherwise, remember this operation and resolve its location later.
-      opsAndArgumentsWithDeferredLocs.emplace_back(opOrArgument, tok);
+      opsWithDeferredLocs.emplace_back(op, tok);
     }
 
     // Otherwise, we parse the location directly.
@@ -1918,12 +1890,8 @@ OperationParser::parseTrailingLocationSpecifier(OpOrArgument opOrArgument) {
   if (parseToken(Token::r_paren, "expected ')' in location"))
     return failure();
 
-  if (directLoc) {
-    if (auto *op = opOrArgument.dyn_cast<Operation *>())
-      op->setLoc(directLoc);
-    else
-      opOrArgument.get<BlockArgument>().setLoc(directLoc);
-  }
+  if (directLoc)
+    op->setLoc(directLoc);
   return success();
 }
 
@@ -1974,8 +1942,7 @@ ParseResult OperationParser::parseRegion(
                    .attachNote(getEncodedSourceLocation(*defLoc))
                << "previously referenced here";
       }
-      auto loc = getEncodedSourceLocation(placeholderArgPair.first.loc);
-      BlockArgument arg = block->addArgument(placeholderArgPair.second, loc);
+      BlockArgument arg = block->addArgument(placeholderArgPair.second);
 
       // Add a definition of this arg to the assembly state if provided.
       if (state.asmState)
@@ -2155,15 +2122,9 @@ ParseResult OperationParser::parseOptionalBlockArgList(Block *owner) {
             if (arg.getType() != type)
               return emitError("argument and block argument type mismatch");
           } else {
-            auto loc = getEncodedSourceLocation(useInfo.loc);
-            arg = owner->addArgument(type, loc);
+            arg = owner->addArgument(type);
           }
 
-          // If the argument has an explicit loc(...) specifier, parse and apply
-          // it.
-          if (parseTrailingLocationSpecifier(arg))
-            return failure();
-
           // Mark this block argument definition in the parser state if it was
           // provided.
           if (state.asmState)

diff  --git a/mlir/test/IR/locations.mlir b/mlir/test/IR/locations.mlir
index e4a46f84fe57..5ad854eedc10 100644
--- a/mlir/test/IR/locations.mlir
+++ b/mlir/test/IR/locations.mlir
@@ -44,25 +44,5 @@ func @escape_strings() {
 // CHECK-ALIAS: "foo.op"() : () -> () loc(#[[LOC:.*]])
 "foo.op"() : () -> () loc(#loc)
 
-// CHECK-LABEL: func @argLocs(
-// CHECK-SAME:  %arg0: i32 loc({{.*}}locations.mlir":[[# @LINE+1]]:15),
-func @argLocs(%x: i32,
-// CHECK-SAME:  %arg1: i64 loc({{.*}}locations.mlir":[[# @LINE+1]]:15))
-              %y: i64 loc("hotdog")) {
-  return
-}
-
-// CHECK-LABEL: "foo.unknown_op_with_bbargs"()
-"foo.unknown_op_with_bbargs"() ({
-// CHECK-NEXT: ^bb0(%arg0: i32 loc({{.*}}locations.mlir":[[# @LINE+1]]:7),
- ^bb0(%x: i32,
-// CHECK-SAME: %arg1: i32 loc("cheetos"),
-      %y: i32 loc("cheetos"),
-// CHECK-SAME: %arg2: i32 loc("out_of_line_location")):
-      %z: i32 loc(#loc)):
-    %1 = addi %x, %y : i32
-    "foo.yield"(%1) : (i32) -> ()
-  }) : () -> ()
-
 // CHECK-ALIAS: #[[LOC]] = loc("out_of_line_location")
 #loc = loc("out_of_line_location")

diff  --git a/mlir/test/Transforms/test-legalize-type-conversion.mlir b/mlir/test/Transforms/test-legalize-type-conversion.mlir
index e7ffb7ae6a3e..9ce69519006a 100644
--- a/mlir/test/Transforms/test-legalize-type-conversion.mlir
+++ b/mlir/test/Transforms/test-legalize-type-conversion.mlir
@@ -1,9 +1,7 @@
 // RUN: mlir-opt %s -test-legalize-type-conversion -allow-unregistered-dialect -split-input-file -verify-diagnostics | FileCheck %s
 
-
-func @test_invalid_arg_materialization(
-  // expected-error at below {{failed to materialize conversion for block argument #0 that remained live after conversion, type was 'i16'}}
-  %arg0: i16) {
+// expected-error at below {{failed to materialize conversion for block argument #0 that remained live after conversion, type was 'i16'}}
+func @test_invalid_arg_materialization(%arg0: i16) {
   // expected-note at below {{see existing live user here}}
   "foo.return"(%arg0) : (i16) -> ()
 }

diff  --git a/mlir/test/mlir-tblgen/pattern.mlir b/mlir/test/mlir-tblgen/pattern.mlir
index affc3d7a9396..6918f319198c 100644
--- a/mlir/test/mlir-tblgen/pattern.mlir
+++ b/mlir/test/mlir-tblgen/pattern.mlir
@@ -37,7 +37,7 @@ func @verifyZeroArg() -> i32 {
 }
 
 // CHECK-LABEL: testIgnoreArgMatch
-// CHECK-SAME: (%{{[a-z0-9]*}}: i32 loc({{[^)]*}}), %[[ARG1:[a-z0-9]*]]: i32 loc({{[^)]*}}),
+// CHECK-SAME: (%{{[a-z0-9]*}}: i32, %[[ARG1:[a-z0-9]*]]: i32
 func @testIgnoreArgMatch(%arg0: i32, %arg1: i32, %arg2: i32, %arg3: f32) {
   // CHECK: "test.ignore_arg_match_dst"(%[[ARG1]]) {f = 15 : i64}
   "test.ignore_arg_match_src"(%arg0, %arg1, %arg2) {d = 42, e = 24, f = 15} : (i32, i32, i32) -> ()
@@ -53,7 +53,7 @@ func @testIgnoreArgMatch(%arg0: i32, %arg1: i32, %arg2: i32, %arg3: f32) {
 }
 
 // CHECK-LABEL: verifyInterleavedOperandAttribute
-// CHECK-SAME:    %[[ARG0:.*]]: i32 loc({{[^)]*}}), %[[ARG1:.*]]: i32 loc({{[^)]*}})
+// CHECK-SAME:    %[[ARG0:.*]]: i32, %[[ARG1:.*]]: i32
 func @verifyInterleavedOperandAttribute(%arg0: i32, %arg1: i32) {
   // CHECK: "test.interleaved_operand_attr2"(%[[ARG0]], %[[ARG1]]) {attr1 = 15 : i64, attr2 = 42 : i64}
   "test.interleaved_operand_attr1"(%arg0, %arg1) {attr1 = 15, attr2 = 42} : (i32, i32) -> ()
@@ -114,7 +114,7 @@ func @verifyAllAttrConstraintOf() -> (i32, i32, i32) {
 }
 
 // CHECK-LABEL: verifyManyArgs
-// CHECK-SAME: (%[[ARG:.*]]: i32 loc({{[^)]*}}))
+// CHECK-SAME: (%[[ARG:.*]]: i32)
 func @verifyManyArgs(%arg: i32) {
   // CHECK: "test.many_arguments"(%[[ARG]], %[[ARG]], %[[ARG]], %[[ARG]], %[[ARG]], %[[ARG]], %[[ARG]], %[[ARG]], %[[ARG]])
   // CHECK-SAME: {attr1 = 24 : i64, attr2 = 42 : i64, attr3 = 42 : i64, attr4 = 42 : i64, attr5 = 42 : i64, attr6 = 42 : i64, attr7 = 42 : i64, attr8 = 42 : i64, attr9 = 42 : i64}


        


More information about the Mlir-commits mailing list