[PATCH] D74276: [mlir][DeclarativeParser] Add support for attributes with buildable types.

[Lei Zhang via Phabricator via llvm-commits]

antiagainst requested changes to this revision.
antiagainst added a comment.
This revision now requires changes to proceed.

It seems the parsers/printers modified in this patch does not use the attribute-type-ellison feature introduced in this patch? Can we touch parsers/printers really uses this feature? Also should we document this? Otherwise it's subtle behavior users might find very confusing. (The modifications to parsers/printers in this patch can be split out.)



================
Comment at: mlir/include/mlir/IR/OpBase.td:632
 
+  // The value type of this attribute.
+  Type valueType = ?;
----------------
Let's put some comments here as for what this field mean and what it's used for. We already have `storageType`, `returnType`. It can be quite confusing for somebody reading the code later. ;)


================
Comment at: mlir/include/mlir/IR/OpBase.td:687
 
-// A generic attribute that must be constructed around a specific type
+// A generic attribute that must be constructed around a specific buildable type
 // `attrValType`. Backed by MLIR attribute kind `attrKind`.
----------------
We removed the `BuildType` requirement but mentioned it in the comment?


================
Comment at: mlir/include/mlir/IR/OpBase.td:1234
   let defaultValue = attr.defaultValue;
+  let valueType = attr.valueType;
   let isOptional = attr.isOptional;
----------------
Should we also pass this through other attribute "decorators" like `OptionalAttr` and `defaultValuedAttr`?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:831
+    Never,
+    /// The type may be elided when it matches the default used in the  parser
+    /// (for example i64 is the default for integer attributes).
----------------
Nit: double space before `parser`. ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74276