[PATCH] D79463: [mlir] Add NamedAttrList

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar marked an inline comment as done.
jpienaar added inline comments.


================
Comment at: mlir/lib/IR/Attributes.cpp:175
                                              MLIRContext *context) {
+  if (value.empty())
+    return get(value, context);
----------------
rriddle wrote:
> Add a comment for why you are doing this.
Updated to getEmpty and now should be more self-documenting, thanks


================
Comment at: mlir/lib/IR/MLIRContext.cpp:751
+                                   MLIRContext *context) {
+  if (value.empty())
+    return context->getImpl().emptyDictionaryAttr;
----------------
rriddle wrote:
> We could just have a private `DictionaryAttr::getEmpty(MLIRContext *context)` that the other methods call. That would allow for moving the other part of this back to Attributes.cpp
Good idea, this cleaned this up.


================
Comment at: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:881
+  // Operands
+  if (numVariadicOperands == 0 || numNonVariadicOperands != 0)
+    body << "  assert(operands.size()"
----------------
rriddle wrote:
> This seems unrelated?
Not really, without this we end up calling a method that has a ArrayRef<NamedAttribute> as operand and so end up still converting back and forth and iterating through the attributes once redundantly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79463