[MLIR] Check for duplicate entries in attribute dictionary during custom parsing

[jurahul]

Summary

• Verify that attributes parsed using a custom parser do not have duplicates.
• If there are duplicated in the attribute dictionary in the input, they get caught during the dictionary parsing.
• This check verifies that there is no duplication between the parsed dictionary and any attributes that might be added by the custom parser (or when the custom parsing code adds duplicate attributes).
• Fixes https://bugs.llvm.org/show_bug.cgi?id=48025

[rriddle]

Why place this in the declarative parser/printer? This(i.e. duplicate attributes) looks like it could apply to any custom op parser, declarative or not.

[jurahul]

By custom op parser, you mean ops that implement their own print and parse functions, right? What do you suggest would be a good place to handle this? In OpBuilder::createOperation() might be too late?

[rriddle]

In D90502#2365738, @jurahul wrote:

By custom op parser, you mean ops that implement their own print and parse functions, right? What do you suggest would be a good place to handle this? In OpBuilder::createOperation() might be too late?

Yeah. Hmm, can we check here(https://github.com/llvm/llvm-project/blob/72ddd559b8aafef402091f8e192e025022e4ebef/mlir/lib/Parser/Parser.cpp#L848)? If the parser didn't emit an error, we should be able to check if the operation state has duplicate attributes.

[jurahul]

Move error checking to CustomOpParser::parseOperation

[jurahul]

Ping.

[rriddle]

This is really really clean Rahul, awesome!

mlir/include/mlir/IR/Attributes.h

295

nit: name within the given array

mlir/lib/IR/Attributes.cpp

130

Missing a static here.

143–145

mlir/lib/IR/OperationSupport.cpp

40–41

[jurahul]

Address comments

mlir/include/mlir/IR/Attributes.h

static bool sort(ArrayRef<NamedAttribute> values,
                    SmallVectorImpl<NamedAttribute> &storage);
 
   /// Sorts the NamedAttributes in the array ordered by name as expected by
   /// getWithSorted in place on an array and returns whether the values needed
   /// to be sorted.
   /// Requires: uniquely named attributes.
   static bool sortInPlace(SmallVectorImpl<NamedAttribute> &array);
 
+  /// Returns an entry with a duplicate name in `array`, if it exists, else
+  /// returns llvm::None. If `isSorted` is true, the array is assumed to be
+  /// sorted else it will be sorted in place before finding the duplicate entry.
+  static Optional<NamedAttribute>
+  findDuplicate(SmallVectorImpl<NamedAttribute> &array, bool isSorted);
+
 private:
   /// Return empty dictionary.
   static DictionaryAttr getEmpty(MLIRContext *context);
 };
 
 //===----------------------------------------------------------------------===//
 // FloatAttr
 //===----------------------------------------------------------------------===//

mlir/include/mlir/IR/OperationSupport.h

public:
   void reserve(size_type N) { attrs.reserve(N); }
 
   /// Add an attribute with the specified name.
   void push_back(NamedAttribute newAttribute);
 
   /// Pop last element from list.
   void pop_back() { attrs.pop_back(); }
 
+  /// Returns an entry with a duplicate name the list, if it exists, else
+  /// returns llvm::None.
+  Optional<NamedAttribute> findDuplicate() const;
+
   /// Return a dictionary attribute for the underlying dictionary. This will
   /// return an empty dictionary attribute if empty rather than null.
   DictionaryAttr getDictionary(MLIRContext *context) const;
 
   /// Return all of the attributes on this operation.
   ArrayRef<NamedAttribute> getAttrs() const;
 
   /// Return the specified attribute if present, null otherwise.

mlir/lib/IR/Attributes.cpp

case 0:
     // Zero already sorted.
     break;
   case 1:
     // One already sorted but may need to be copied.
     if (!inPlace)
       storage.assign({value[0]});
     break;
   case 2: {
-    assert(value[0].first != value[1].first &&
-           "DictionaryAttr element names must be unique");
     bool isSorted = value[0] < value[1];
     if (inPlace) {
       if (!isSorted)
         std::swap(storage[0], storage[1]);
     } else if (isSorted) {
       storage.assign({value[0], value[1]});
     } else {
       storage.assign({value[1], value[0]});
     }
     return !isSorted;
   }
   default:
     if (!inPlace)
       storage.assign(value.begin(), value.end());
     // Check to see they are sorted already.
     bool isSorted = llvm::is_sorted(value);
     if (!isSorted) {
       // If not, do a general sort.
       llvm::array_pod_sort(storage.begin(), storage.end());
       value = storage;
     }
-
-    // Ensure that the attribute elements are unique.
-    assert(std::adjacent_find(value.begin(), value.end(),
-                              [](NamedAttribute l, NamedAttribute r) {
-                                return l.first == r.first;
-                              }) == value.end() &&
-           "DictionaryAttr element names must be unique");
     return !isSorted;
   }
   return false;
 }
 
+/// Returns an entry with a duplicate name from the given sorted array of named
+/// attributes. Returns llvm::None if all elements have unique names.
+static Optional<NamedAttribute>
+findDuplicateElement(ArrayRef<NamedAttribute> value) {
+  const Optional<NamedAttribute> none{llvm::None};
+  if (value.size() < 2)
+    return none;
+
+  if (value.size() == 2)
+    return value[0].first == value[1].first ? value[0] : none;
+
+  auto it = std::adjacent_find(
+      value.begin(), value.end(),
+      [](NamedAttribute l, NamedAttribute r) { return l.first == r.first; });
+  return it != value.end() ? *it : none;
+}
+
 bool DictionaryAttr::sort(ArrayRef<NamedAttribute> value,
                           SmallVectorImpl<NamedAttribute> &storage) {
-  return dictionaryAttrSort</*inPlace=*/false>(value, storage);
+  bool isSorted = dictionaryAttrSort</*inPlace=*/false>(value, storage);
+  assert(!findDuplicateElement(storage) &&
+         "DictionaryAttr element names must be unique");
+  return isSorted;
 }
 
 bool DictionaryAttr::sortInPlace(SmallVectorImpl<NamedAttribute> &array) {
-  return dictionaryAttrSort</*inPlace=*/true>(array, array);
+  bool isSorted = dictionaryAttrSort</*inPlace=*/true>(array, array);
+  assert(!findDuplicateElement(array) &&
+         "DictionaryAttr element names must be unique");
+  return isSorted;
+}
+
+Optional<NamedAttribute>
+DictionaryAttr::findDuplicate(SmallVectorImpl<NamedAttribute> &array,
+                              bool isSorted) {
+  if (!isSorted)
+    dictionaryAttrSort</*inPlace=*/true>(array, array);
+  return findDuplicateElement(array);
 }
 
 DictionaryAttr DictionaryAttr::get(ArrayRef<NamedAttribute> value,
                                    MLIRContext *context) {
   if (value.empty())
     return DictionaryAttr::getEmpty(context);
   assert(llvm::all_of(value,
                       [](const NamedAttribute &attr) { return attr.second; }) &&
          "value cannot have null entries");
 
   // We need to sort the element list to canonicalize it.
   SmallVector<NamedAttribute, 8> storage;
   if (dictionaryAttrSort</*inPlace=*/false>(value, storage))
     value = storage;
-
+  assert(!findDuplicateElement(value) &&
+         "DictionaryAttr element names must be unique");
   return Base::get(context, value);
 }
 /// Construct a dictionary with an array of values that is known to already be
 /// sorted by name and uniqued.
 DictionaryAttr DictionaryAttr::getWithSorted(ArrayRef<NamedAttribute> value,
                                              MLIRContext *context) {
   if (value.empty())
     return DictionaryAttr::getEmpty(context);
   // Ensure that the attribute elements are unique and sorted.
   assert(llvm::is_sorted(value,
                          [](NamedAttribute l, NamedAttribute r) {
                            return l.first.strref() < r.first.strref();
                          }) &&
          "expected attribute values to be sorted");
-  assert(std::adjacent_find(value.begin(), value.end(),
-                            [](NamedAttribute l, NamedAttribute r) {
-                              return l.first == r.first;
-                            }) == value.end() &&
+  assert(!findDuplicateElement(value) &&
          "DictionaryAttr element names must be unique");
   return Base::get(context, value);
 }
 
 ArrayRef<NamedAttribute> DictionaryAttr::getValue() const {
   return getImpl()->getElements();
 }
 

mlir/lib/IR/OperationSupport.cpp

 }
 
 NamedAttrList::NamedAttrList(const_iterator in_start, const_iterator in_end) {
   assign(in_start, in_end);
 }
 
 ArrayRef<NamedAttribute> NamedAttrList::getAttrs() const { return attrs; }
 
+Optional<NamedAttribute> NamedAttrList::findDuplicate() const {
+  Optional<NamedAttribute> duplicate =
+      DictionaryAttr::findDuplicate(attrs, isSorted());
+  // DictionaryAttr::findDuplicate will sort the list, so reset the sorted
+  // state.
+  if (!isSorted())
+    dictionarySorted.setPointerAndInt(nullptr, true);
+  return duplicate;
+}
+
 DictionaryAttr NamedAttrList::getDictionary(MLIRContext *context) const {
   if (!isSorted()) {
     DictionaryAttr::sortInPlace(attrs);
     dictionarySorted.setPointerAndInt(nullptr, true);
   }
   if (!dictionarySorted.getPointer())
     dictionarySorted.setPointer(DictionaryAttr::getWithSorted(attrs, context));
   return dictionarySorted.getPointer().cast<DictionaryAttr>();

mlir/lib/Parser/AttributeParser.cpp

auto parseElt = [&]() -> ParseResult {
     if (getToken().is(Token::string))
       nameId = builder.getIdentifier(getToken().getStringValue());
     else if (getToken().isAny(Token::bare_identifier, Token::inttype) ||
              getToken().isKeyword())
       nameId = builder.getIdentifier(getTokenSpelling());
     else
       return emitError("expected attribute name");
     if (!seenKeys.insert(*nameId).second)
-      return emitError("duplicate key in dictionary attribute");
+      return emitError("duplicate key '")
+             << *nameId << "' in dictionary attribute";
     consumeToken();
 
     // Lazy load a dialect in the context if there is a possible namespace.
     auto splitName = nameId->strref().split('.');
     if (!splitName.second.empty())
       getContext()->getOrLoadDialect(splitName.first);
 
     // Try to parse the '=' for the attribute value.

mlir/lib/Parser/Parser.cpp

CustomOpAsmParser(SMLoc nameLoc,
       : nameLoc(nameLoc), resultIDs(resultIDs), opDefinition(opDefinition),
         parser(parser) {}
 
   /// Parse an instance of the operation described by 'opDefinition' into the
   /// provided operation state.
   ParseResult parseOperation(OperationState &opState) {
     if (opDefinition->parseAssembly(*this, opState))
       return failure();
+    // Verify that the parsed attributes does not have duplicate attributes.
+    // This can happen if an attribute set during parsing is also specified in
+    // the attribute dictionary in the assembly, or the attribute is set
+    // multiple during parsing.
+    Optional<NamedAttribute> duplicate = opState.attributes.findDuplicate();
+    if (duplicate)
+      return emitError(getNameLoc(), "attribute '")
+             << duplicate->first
+             << "' occurs more than once in the attribute list";
     return success();
   }
 
   Operation *parseGenericOperation(Block *insertBlock,
                                    Block::iterator insertPt) final {
     return parser.parseGenericOperation(insertBlock, insertPt);
   }
 

mlir/test/IR/invalid.mlir

"test.out_of_range_attribute"() {
     attr = 39614081257132168796771975168 : si96
   } : () -> ()
   return
 }
 
 // -----
 
 func @duplicate_dictionary_attr_key() {
-  // expected-error @+1 {{duplicate key in dictionary attribute}}
+  // expected-error @+1 {{duplicate key 'a' in dictionary attribute}}
   "foo.op"() {a, a} : () -> ()
 }
 
 // -----
 
+// expected-error @+1 {{attribute 'attr' occurs more than once in the attribute list}}
+test.format_symbol_name_attr_op @name { attr = "xx" }
+
+// -----
+
 func @forward_reference_type_check() -> (i8) {
   br ^bb2
 
 ^bb1:
   // expected-note @+1 {{previously used here with type 'i8'}}
   return %1 : i8
 
 ^bb2: