Use consistent spacing before custom directives for op and attr/type assemblyFormat.

[GleasonK]

Summary

Currently, assemblyFormat custom<A>($a) custom<B>($b) has different spacing
if used for Ops vs Attrs/Types. Ops insert a space if needed before the custom directive,
while attributes and types do not.

This leads to the following two patterns in attributes / types:

# 1. Whitespace literal
let assemblyFormat = "... ` ` custom<A>($a)"


# 2. Custom printer code includes spacing
void printB(...) {
  printer << ' ' << b;
}

Moving this spacing into the generated code allows for some cleanup in mlir and
improves the consistency of custom directives.

[GleasonK]

Relatively new to the LLVM community, and unsure of review conventions. I see this is marked as" accepted and ready to land", unsure if I am still waiting on feedback from remaining reviewers.

If not, could someone with commit access land this for me? I rebased and built MLIR locally, changes don't appear to be stale yet.

[Mogball]

Apologies for the delay. Landing

mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.td

let description = [{
 
     ```mlir
     !llvm.array<4 x i32>
     ```
   }];
 
   let parameters = (ins "Type":$elementType, "unsigned":$numElements);
   let assemblyFormat = [{
-    `<` $numElements `x` ` ` custom<PrettyLLVMType>($elementType) `>`
+    `<` $numElements `x` custom<PrettyLLVMType>($elementType) `>`
   }];
 
   let genVerifyDecl = 1;
 
   let builders = [
     TypeBuilderWithInferredContext<(ins "Type":$elementType,
                                         "unsigned":$numElements)>
   ];
def LLVMFixedVectorType : LLVMType<"LLVMFixedVector", "vec", [
   let description = [{
     LLVM dialect scalable vector type, represents a sequence of elements of
     unknown length that is known to be divisible by some constant. These
     elements can be processed as one in SIMD context.
   }];
 
   let parameters = (ins "Type":$elementType, "unsigned":$numElements);
   let assemblyFormat = [{
-    `<` $numElements `x` ` ` custom<PrettyLLVMType>($elementType) `>`
+    `<` $numElements `x` custom<PrettyLLVMType>($elementType) `>`
   }];
 
   let genVerifyDecl = 1;
 
   let builders = [
     TypeBuilderWithInferredContext<(ins "Type":$elementType,
                                         "unsigned":$numElements)>
   ];

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

 }
 def TestArrayOfEnums : ArrayOfAttr<Test_Dialect, "ArrayOfEnums",
     "array_of_enums", "SimpleEnumAttr">;
 
 // Test custom directive as optional group anchor.
 def TestCustomAnchor : Test_Attr<"TestCustomAnchor"> {
   let parameters = (ins "int":$a, OptionalParameter<"mlir::Optional<int>">:$b);
   let mnemonic = "custom_anchor";
-  let assemblyFormat = "`<` $a (`>`) : (`,` ` ` custom<TrueFalse>($b)^ `>`)?";
+  let assemblyFormat = "`<` $a (`>`) : (`,` custom<TrueFalse>($b)^ `>`)?";
 }
 
 def Test_IteratorTypeEnum
     : EnumAttr<Test_Dialect, IteratorType, "iterator_type"> {
   let assemblyFormat = "`<` $value `>`";
 }
 
 def Test_IteratorTypeArrayAttr
     : TypedArrayAttrBase<Test_IteratorTypeEnum,
   "Iterator type should be an enum.">;
 
 
 #endif // TEST_ATTRDEFS

mlir/test/lib/Dialect/Test/TestDialect.cpp

static ParseResult parseCustomDirectiveAttributes(OpAsmParser &parser,
   if (parser.parseAttribute(attr))
     return failure();
   if (succeeded(parser.parseOptionalComma())) {
     if (parser.parseAttribute(optAttr))
       return failure();
   }
   return success();
 }
-
+static ParseResult parseCustomDirectiveSpacing(OpAsmParser &parser,
+                                               mlir::StringAttr &attr) {
+  return parser.parseAttribute(attr);
+}
 static ParseResult parseCustomDirectiveAttrDict(OpAsmParser &parser,
                                                 NamedAttrList &attrs) {
   return parser.parseOptionalAttrDict(attrs);
 }
 static ParseResult parseCustomDirectiveOptionalOperandRef(
     OpAsmParser &parser, Optional<OpAsmParser::UnresolvedOperand> &optOperand) {
   int64_t operandCount = 0;
   if (parser.parseInteger(operandCount))

 }
 static void printCustomDirectiveAttributes(OpAsmPrinter &printer, Operation *,
                                            Attribute attribute,
                                            Attribute optAttribute) {
   printer << attribute;
   if (optAttribute)
     printer << ", " << optAttribute;
 }
-
+static void printCustomDirectiveSpacing(OpAsmPrinter &printer, Operation *op,
+                                        Attribute attribute) {
+  printer << attribute;
+}
 static void printCustomDirectiveAttrDict(OpAsmPrinter &printer, Operation *op,
                                          DictionaryAttr attrs) {
   printer.printOptionalAttrDict(attrs.getValue());
 }
 
 static void printCustomDirectiveOptionalOperandRef(OpAsmPrinter &printer,
                                                    Operation *op,
                                                    Value optOperand) {

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

def FormatCustomDirectiveAttributes
   let assemblyFormat = [{
     custom<CustomDirectiveAttributes>(
       $attr, $optAttr
     )
     attr-dict
   }];
 }
 
+def FormatCustomDirectiveSpacing
+    : TEST_Op<"format_custom_directive_spacing"> {
+  let arguments = (ins StrAttr:$attr1, StrAttr:$attr2);
+  let assemblyFormat = [{
+    custom<CustomDirectiveSpacing>($attr1)
+    custom<CustomDirectiveSpacing>($attr2)
+    attr-dict
+  }];
+}
+
 def FormatCustomDirectiveAttrDict
     : TEST_Op<"format_custom_directive_attrdict"> {
   let arguments = (ins I64Attr:$attr, OptionalAttr<I64Attr>:$optAttr);
   let assemblyFormat = [{
     custom<CustomDirectiveAttrDict>( attr-dict )
   }];
 }
 

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

def TestTypeDefaultValuedType : Test_Type<"TestTypeDefaultValuedType"> {
   );
   let mnemonic = "default_valued_type";
   let assemblyFormat = "`<` (`(` $type^ `)`)? `>`";
 }
 
 def TestTypeCustom : Test_Type<"TestTypeCustom"> {
   let parameters = (ins "int":$a, OptionalParameter<"mlir::Optional<int>">:$b);
   let mnemonic = "custom_type";
-  let assemblyFormat = [{ `<` custom<CustomTypeA>($a)
+  let assemblyFormat = [{ `<` custom<CustomTypeA>($a) ``
                               custom<CustomTypeB>(ref($a), $b) `>` }];
 }
 
+def TestTypeCustomSpacing : Test_Type<"TestTypeCustomSpacing"> {
+  let parameters = (ins "int":$a, "int":$b);
+  let mnemonic = "custom_type_spacing";
+  let assemblyFormat = [{ `<` custom<CustomTypeA>($a)
+                              custom<CustomTypeA>($b) `>` }];
+}
+
 def TestTypeCustomString : Test_Type<"TestTypeCustomString"> {
   let parameters = (ins StringRefParameter<>:$foo);
   let mnemonic = "custom_type_string";
   let assemblyFormat = [{ `<` custom<FooString>($foo)
                               custom<BarString>(ref($foo)) `>` }];
 }
 
 def TestTypeElseAnchor : Test_Type<"TestTypeElseAnchor"> {

mlir/test/lib/Dialect/Test/TestTypes.cpp

static void printFooString(AsmPrinter &printer, StringRef foo) {
   printer << '"' << foo << '"';
 }
 
 static LogicalResult parseBarString(AsmParser &parser, StringRef foo) {
   return parser.parseKeyword(foo);
 }
 
 static void printBarString(AsmPrinter &printer, StringRef foo) {
-  printer << ' ' << foo;
+  printer << foo;
 }
 //===----------------------------------------------------------------------===//
 // Tablegen Generated Definitions
 //===----------------------------------------------------------------------===//
 
 #define GET_TYPEDEF_CLASSES
 #include "TestTypeDefs.cpp.inc"
 

mlir/test/mlir-tblgen/attr-or-type-format-roundtrip.mlir

 // CHECK: !test.spaces< 5
 // CHECK-NEXT: ()() 6>
 // CHECK: !test.ap_float<5.000000e+00>
 // CHECK: !test.ap_float<>
 // CHECK: !test.default_valued_type<(i64)>
 // CHECK: !test.default_valued_type<>
 // CHECK: !test.custom_type<-5>
 // CHECK: !test.custom_type<2 0 1 5>
+// CHECK: !test.custom_type_spacing<1 2>
 // CHECK: !test.custom_type_string<"foo" foo>
 // CHECK: !test.custom_type_string<"bar" bar>
 
 func.func private @test_roundtrip_default_parsers_struct(
   !test.no_parser<255, [1, 2, 3, 4, 5], "foobar", 4>
 ) -> (
   !test.struct_capture_all<v3 = 3, v1 = 1, v2 = 2, v0 = 0>,
   !test.optional_param<, 6>,
) -> (
   !test.optional_group_struct<(b = 5, a = 10)>,
   !test.spaces<5 ()() 6>,
   !test.ap_float<5.0>,
   !test.ap_float<>,
   !test.default_valued_type<(i64)>,
   !test.default_valued_type<>,
   !test.custom_type<-5>,
   !test.custom_type<2 9 9 5>,
+  !test.custom_type_spacing<1 2>,
   !test.custom_type_string<"foo" foo>,
   !test.custom_type_string<"bar" bar>
 )

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

 ^bb1:
   // CHECK: test.format_custom_directive_successors ^bb2
   test.format_custom_directive_successors ^bb2
 
 ^bb2:
   return
 }
 
+// CHECK: test.format_custom_directive_spacing "a" "b"
+test.format_custom_directive_spacing "a" "b"
+
 // CHECK: test.format_literal_following_optional_group(5 : i32) : i32 {a}
 test.format_literal_following_optional_group(5 : i32) : i32 {a}
 
 func.func @variadic(%a: i32) {
   // CHECK: test.ellipsis(%{{.*}} ...) : i32 ...
   test.ellipsis(%a ...) : i32 ...
   return
 }

mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp

void DefFormat::genStructPrinter(StructDirective *el, FmtContext &ctx,
   genCommaSeparatedPrinter(
       llvm::to_vector(el->getParams()), ctx, os, [&](ParameterElement *param) {
         os << tgfmt("$_printer << \"$0 = \";\n", &ctx, param->getName());
       });
 }
 
 void DefFormat::genCustomPrinter(CustomDirective *el, FmtContext &ctx,
                                  MethodBody &os) {
+  // Insert a space before the custom directive, if necessary.
+  if (shouldEmitSpace || !lastWasPunctuation)
+    os << tgfmt("$_printer << ' ';\n", &ctx);
+  shouldEmitSpace = true;
+  lastWasPunctuation = false;
+
   os << tgfmt("print$0($_printer", &ctx, el->getName());
   os.indent();
   for (FormatElement *arg : el->getArguments()) {
     os << ",\n";
     if (auto *param = dyn_cast<ParameterElement>(arg)) {
       os << param->getParam().getAccessorName() << "()";
     } else if (auto *ref = dyn_cast<RefDirective>(arg)) {
       os << cast<ParameterElement>(ref->getArg())->getParam().getAccessorName()