[MLIR] Check for duplicate entries in attribute dictionary during cus…

…tom parsing

- 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

Differential Revision: https://reviews.llvm.org/D90502

mlir/include/mlir/IR/Attributes.h

@@ -292,6 +292,12 @@ class DictionaryAttr
   /// 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);

mlir/include/mlir/IR/OperationSupport.h

@@ -262,6 +262,10 @@ class NamedAttrList {
   /// 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;

mlir/lib/IR/Attributes.cpp

@@ -99,8 +99,6 @@ static bool dictionaryAttrSort(ArrayRef<NamedAttribute> value,
       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)
@@ -122,25 +120,49 @@ static bool dictionaryAttrSort(ArrayRef<NamedAttribute> value,
       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,
@@ -155,7 +177,8 @@ DictionaryAttr DictionaryAttr::get(ArrayRef<NamedAttribute> value,
   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
@@ -170,10 +193,7 @@ DictionaryAttr DictionaryAttr::getWithSorted(ArrayRef<NamedAttribute> value,
                            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);
 }

mlir/lib/IR/OperationSupport.cpp

@@ -32,6 +32,16 @@ NamedAttrList::NamedAttrList(const_iterator in_start, const_iterator 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);

mlir/lib/Parser/AttributeParser.cpp

@@ -253,7 +253,8 @@ ParseResult Parser::parseAttributeDict(NamedAttrList &attributes) {
     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.

mlir/lib/Parser/Parser.cpp

@@ -846,6 +846,15 @@ class CustomOpAsmParser : public OpAsmParser {
   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();
   }
 

mlir/test/IR/invalid.mlir

@@ -1513,12 +1513,17 @@ func @really_large_bound() {
 // -----
 
 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