[PATCH] D79463: [mlir] Add NamedAttrList

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/include/mlir/IR/Attributes.h:1705
+inline ::llvm::hash_code hash_value(const MutableDictionaryAttr &arg) {
+  if (!arg.attrs || arg.attrs.empty())
+    return ::llvm::hash_value((void *)nullptr);
----------------
When would it be .empty() here if we are using nullptr for empty?


================
Comment at: mlir/include/mlir/IR/Attributes.h:1707
+    return ::llvm::hash_value((void *)nullptr);
+  return hash_value(arg.attrs.cast<Attribute>());
+}
----------------
The .cast<Attribute> seems unnecessary.


================
Comment at: mlir/include/mlir/IR/FunctionSupport.h:581
   SmallString<8> nameOut;
-  if (auto newAttr = attributes.getDictionary())
+  if (attributes.getAttrs().empty()) {
+    this->getOperation()->removeAttr(getResultAttrName(index, nameOut));
----------------
attributes.empty()?


================
Comment at: mlir/include/mlir/IR/FunctionSupport.h:585
+    auto newAttr = attributes.getDictionary(
+        attributes.getAttrs().front().second.getContext());
     return this->getOperation()->setAttr(getResultAttrName(index, nameOut),
----------------
this->getOperation()->getContext()?


================
Comment at: mlir/lib/IR/MLIRContext.cpp:749
 
+/// Return empty context.
+DictionaryAttr DictionaryAttr::getEmpty(MLIRContext *context) {
----------------
context -> dictionary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79463