Add a cppType string in AttrDef to make it possible to use them as parameters in other attributes

[rriddle]

mlir/test/lib/Dialect/Test/TestAttrDefs.td

48–51

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

Why do you need an op for this?

[mehdi_amini]

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

This is to check that attribute when printed in the context of the assembly format generated by an operation. We have quite some hole in test around this (actually I need to add more test around attribute aliases around this)

[rriddle]

mlir/include/mlir/IR/OpBase.td

2941–2943

Actually, why is this necessary?

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

I'm not sure I understand, the attribute would be formatted the exact same way. I'm not sure I would want to test every possible matrix of context, especially given that they all go through the same code path.

[mehdi_amini]

Oh we crossed, I was rebasing and pushed while you marked it as "requested change",

mlir/include/mlir/IR/OpBase.td

2941–2943

TableGen crashes without it, it is used to construct the Builder for the generated entity (Attribute or Type)

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

Actually they don't really go through the same code path, I learnt it the hard way while changing a bunch of things.

The different is subtle but exists: when you come from the generic parser you will always go through the Dialect::printAttribute as you have a type-erased Attribute instance.
When you are in the assembly parser for a given operation, the attribute instance is the concrete attribute and the generated code dispatch on a non-type-erased class.

[rriddle]

In D113737#3126521, @mehdi_amini wrote:

Oh we crossed, I was rebasing and pushed while you marked it as "requested change",

Ah, it's fine. You gave me context offline. I think we should improve the general ODS concept of cppType though, feels not well baked right now.

[rriddle]

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

That is a tad bit surprising. I would expect that we go directly through the generic parser(parseAttribute)/printer(printAttribute) by default, what does it generate then?

[mehdi_amini]

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

It does end up in the same place... for now.
This is a bit fragile because we generate something like that: parser.parseAttribute(attr) (attr is an output argument here). And so depending on what is parser and the overload you can change on parseAttribute you "could" resolve differently (I'll upload a revision showing this soon-ish).

The issue if we don't duplicate the testing right now, it means that touching at the assembly-format infrastructure in any way is basically not covered by tests (when playing with it I frequently get all of MLIR tests passing but hit issues in IREE or MHLO)

[rriddle]

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

So this is guarding against a hypothetical future change? I'm slightly skeptical of allowing/having different overloaded behavior for printAttribute. Why wouldn't we just expose a different API if we wanted some different form of parsing behavior?

[mehdi_amini]

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

Sure, that's actually what I implemented, but it's not changing the fundamental problem: when I change the assembly format generator I still have all the tests passing by lack of coverage for them.

[rriddle]

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

I'm not sure I understand you here. I don't think we should write a test for every possible format in every possible caller of printAttribute. It isn't adding any additional test coverage if it's going through the same code path.

[mehdi_amini]

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

I don't see this as coverage for printAttribute but as coverage for the declarative assembly format.

The issue I have today is that I can change the declarative assembly format generator for an op, and it won't break any test because we don't have complete round-trip coverage.

[mehdi_amini]

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

It's a bit like if you write a new op with custom assembly by hand and you wouldn't provide test for your custom syntax parsing/printing (round-tripping really).
Even though the C++ you write is calling "printAttribute" right now you'd want a test to ensure that it fits all together and that the printer and parser are in sync.

[rriddle]

mlir/test/lib/Dialect/Test/TestOps.td

1905–1908

I don't see how this test supports that viewpoint? We have tests that checks if an attribute is used in the format, I don't see how this test adds coverage for anything that isn't already covered.

mlir/include/mlir/IR/OpBase.td

: DialectAttr<dialect, CPred<"">, /*descr*/"">,
       AttrOrTypeDef<"Attr", name, traits, baseCppClass> {
   // The name of the C++ Attribute class.
   string cppClassName = name # "Attr";
   let storageType = dialect.cppNamespace # "::" # name # "Attr";
 
   // The underlying C++ value type
   let returnType = dialect.cppNamespace # "::" # cppClassName;
 
+  // Make it possible to use such attributes as parameters for other attributes.
+  string cppType = dialect.cppNamespace # "::" # cppClassName;
+
   // The call expression to convert from the storage type to the return
   // type. For example, an enum can be stored as an int but returned as an
   // enum class.
   //
   // Format: $_self will be expanded to the attribute.
   //
   // For example, `$_self.getValue().getSExtValue()` for `IntegerAttr val` will
   // expand to `getAttrOfType<IntegerAttr>("val").getValue().getSExtValue()`.

mlir/test/lib/Dialect/Test/TestAttrDefs.td

let parameters = (
     // This is special syntax since ArrayRefs require allocation in the
     // constructor.
     ArrayRefParameter<
       "int", // The parameter C++ type.
       "An example of an array of ints" // Parameter description.
       >: $arrayOfInts
   );
 }
+def CompoundAttrNested : Test_Attr<"CompoundAttrNested"> {
+  let mnemonic = "cmpnd_nested";
+  let parameters = (ins CompoundAttrA : $nested );
+  let assemblyFormat = "`<` `nested` `=` $nested `>`";
+}
 
 // An attribute testing AttributeSelfTypeParameter.
 def AttrWithSelfTypeParam : Test_Attr<"AttrWithSelfTypeParam"> {
   let mnemonic = "attr_with_self_type_param";
   let parameters = (ins AttributeSelfTypeParameter<"">:$type);
 }
 
 // An attribute testing AttributeSelfTypeParameter.
 def AttrWithTypeBuilder : Test_Attr<"AttrWithTypeBuilder"> {

mlir/test/lib/Dialect/Test/TestOps.td

def FormatOptionalWithElse : TEST_Op<"format_optional_else"> {
   let assemblyFormat = "(`then` $isFirstBranchPresent^):(`else`)? attr-dict";
 }
 
 def FormatCompoundAttr : TEST_Op<"format_compound_attr"> {
   let arguments = (ins CompoundAttrA:$compound);
   let assemblyFormat = "$compound attr-dict-with-keyword";
 }
 
+def FormatNestedAttr : TEST_Op<"format_nested_attr"> {
+  let arguments = (ins CompoundAttrNested:$nested);
+  let assemblyFormat = "$nested attr-dict-with-keyword";
+}
+
 //===----------------------------------------------------------------------===//
 // Custom Directives
 
 def FormatCustomDirectiveOperands
     : TEST_Op<"format_custom_directive_operands", [AttrSizedOperandSegments]> {
   let arguments = (ins I64:$operand, Optional<I64>:$optOperand,
                        Variadic<I64>:$varOperands);
   let assemblyFormat = [{

mlir/test/mlir-tblgen/op-format.mlir

 
 //===----------------------------------------------------------------------===//
 // Format a custom attribute
 //===----------------------------------------------------------------------===//
 
 // CHECK: test.format_compound_attr #test.cmpnd_a<1, !test.smpla, [5, 6]>
 test.format_compound_attr #test.cmpnd_a<1, !test.smpla, [5, 6]>
 
+// CHECK:  module attributes {test.nested = #test.cmpnd_nested<nested = #test.cmpnd_a<1, !test.smpla, [5, 6]>>} {
+module attributes {test.nested = #test.cmpnd_nested<nested = #test.cmpnd_a<1, !test.smpla, [5, 6]>>} {
+}
+
+// CHECK: test.format_nested_attr #test.cmpnd_nested<nested = #test.cmpnd_a<1, !test.smpla, [5, 6]>>
+test.format_nested_attr #test.cmpnd_nested<nested = #test.cmpnd_a<1, !test.smpla, [5, 6]>>
+
+
 //===----------------------------------------------------------------------===//
 // Format custom directives
 //===----------------------------------------------------------------------===//
 
 // CHECK: test.format_custom_directive_operands %[[I64]], %[[I64]] -> (%[[I64]])
 test.format_custom_directive_operands %i64, %i64 -> (%i64)
 
 // CHECK: test.format_custom_directive_operands %[[I64]] -> (%[[I64]])