[PATCH] D79463: [mlir] Add NamedAttrList

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/IR/Attributes.h:311
   /// Sorts the NamedAttributes in the array ordered by name as expected by
-  /// getWithSorted.
+  /// getWithSorted and returns whether the values needed to be sorted.
   /// Requires: uniquely named attributes.
----------------
nit: whether the values were sorted?


================
Comment at: mlir/include/mlir/IR/FunctionSupport.h:521
+  if (attributes.getAttrs().empty()) {
+    static_cast<ConcreteType *>(this)->removeAttr(
+        getArgAttrName(index, nameOut));
----------------
Can we just add the overload for Operation::removeAttr that takes a string?


================
Comment at: mlir/lib/IR/Attributes.cpp:129
+    } else {
+      if (isSorted)
+        storage.assign({value[0], value[1]});
----------------
nit: merge the if with the previous else


================
Comment at: mlir/lib/IR/Attributes.cpp:175
                                              MLIRContext *context) {
+  if (value.empty())
+    return get(value, context);
----------------
Add a comment for why you are doing this.


================
Comment at: mlir/lib/IR/MLIRContext.cpp:394
+  impl->emptyDictionaryAttr = AttributeUniquer::get<DictionaryAttr>(
+      this, StandardAttributes::Dictionary, ArrayRef<NamedAttribute>{});
 }
----------------
ArrayRef<NamedAttribute>()


================
Comment at: mlir/lib/IR/OperationSupport.cpp:38
+    DictionaryAttr::sortInPlace(attrs);
+    dictionarySorted.setInt(true);
+    dictionarySorted.setPointer(nullptr);
----------------
setPointerAndInt


================
Comment at: mlir/lib/IR/OperationSupport.cpp:71
+  // end of current list.
+  dictionarySorted.setInt(false);
+  dictionarySorted.setPointer(nullptr);
----------------
setPointerAndInt, here and a few other places


================
Comment at: mlir/lib/IR/OperationSupport.cpp:84
+void NamedAttrList::push_back(NamedAttribute newAttribute) {
+  dictionarySorted.setInt(
+      dictionarySorted.getInt() &&
----------------
nit: Can you add a private 'isSorted' method?
also:

if (isSorted())
  dictionarySorted.setInt(attrs.empty() || ...)

?


================
Comment at: mlir/lib/IR/OperationSupport.cpp:94
+Attribute NamedAttrList::get(StringRef name) const {
+  const auto *it = llvm::find_if(
+      attrs, [name](NamedAttribute attr) { return attr.first == name; });
----------------
I would have expected a lower_bound if it was sorted? Also, can we add a static method to deduplicate all of these?


================
Comment at: mlir/lib/IR/OperationSupport.cpp:137
+  it = llvm::lower_bound(
+      attrs, name, [](const NamedAttribute &attr, StringRef name) {
+        return strncmp(attr.first.data(), name.data(), name.size()) < 0;
----------------
Can we add public operator< for NamedAttribute and NamedAttribute/StringRef, likely also Identifier and Identifier/StringRef.


================
Comment at: mlir/lib/IR/OperationSupport.cpp:556
+      op->getName(),
+      op->getAttrs().empty() ? nullptr : op->getAttrDictionary());
 
----------------
We should have MutableDictionaryAttr be directly hashable so we don't need this here.


================
Comment at: mlir/lib/Pass/IRPrinting.cpp:36
       //   - Attributes
-      addDataToHash(
-          hasher,
-          op->getMutableAttrDict().getDictionary().getAsOpaquePointer());
+      addDataToHash(hasher, op->getAttrs().empty()
+                                ? nullptr
----------------
Same here. We could generate a hash to add here instead of using the pointer.


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:881
+  // Operands
+  if (numVariadicOperands == 0 || numNonVariadicOperands != 0)
+    body << "  assert(operands.size()"
----------------
This seems unrelated?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79463/new/

https://reviews.llvm.org/D79463