[PATCH] D78434: [mlir] resolve types from attributes in assemblyFormat

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/test/mlir-tblgen/op-format.mlir:106
+// CHECK: test.format_all_types_match_attr 1 : i64, %[[I64]]
+%ignored_res2 = test.format_all_types_match_attr 1 : i64, %i64
+
----------------
tali wrote:
> I would really like to be able to format this example as `test.format_all_types_match_attr 1, %i64 : i64`, with the type at the end.
> We could add support for `type($attribute)`, and suppress printing of the attribute type if it is specified explicitly or when it can be inferred.
The type of the attribute is required to be resolved when parsing it(i.e. at creation time). You would need some fairly fundamental changes to attributes to support that.


================
Comment at: mlir/tools/mlir-tblgen/OpFormatGen.cpp:735
       continue;
     // Ensure that we don't verify the same variables twice.
     const NamedTypeConstraint *variable = resolver.getVariable();
----------------
tali wrote:
> rriddle wrote:
> > rriddle wrote:
> > > We will also need to add verification of the attribute here if the transformer is non-trivial.
> > Although I suspect 99% of the cases are already handled because we check the storage type when parsing, happy to leave this as a followup for now.
> Which checks would you find useful here?
I mean that if the transformer is dependent on specific invariants of the attribute constraint, the attribute has to be verified first otherwise the transformer could assert/crash. This is the same reasoning as to why the operand/result variables get verified here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78434