[mlir] Allow space literals (` `) in assemblyFormat.

[csigg]

Summary

Spaces are only printed, not parsed.

[rriddle]

Thanks! A few missing cases, but otherwise looks good.

mlir/tools/mlir-tblgen/OpFormatGen.cpp

2243–2245

This should include the SpaceElement as well.

2567

nit: Can you move this comment before the if?

2686–2688

This Case should also include the SpaceElement.

[csigg]

Apply reviewer comments.

mlir/tools/mlir-tblgen/OpFormatGen.cpp

2243–2245

Thanks, I wouldn't have found this or the one below. Is there a simple way to test this?

[csigg]

Add tests for cases added in previous diff.

[rriddle]

Looks good, thanks!

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

 }
 
 //===----------------------------------------------------------------------===//
 // Test Op Asm Format
 //===----------------------------------------------------------------------===//
 
 def FormatLiteralOp : TEST_Op<"format_literal_op"> {
   let assemblyFormat = [{
-    `keyword_$.` `->` `:` `,` `=` `<` `>` `(` `)` `[` `]` attr-dict
+    `keyword_$.` `->` `:` `,` `=` `<` `>` `(` `)` `[` `]` ` ` `(` ` ` `)` ` ` attr-dict
   }];
 }
 
 // Test that we elide attributes that are within the syntax.
 def FormatAttrOp : TEST_Op<"format_attr_op"> {
   let arguments = (ins I64Attr:$attr);
   let assemblyFormat = "$attr attr-dict";
 }

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

 }]>;
 // CHECK: error: unexpected end of file in literal
 // CHECK: error: expected directive, literal, variable, or optional group
 def LiteralInvalidB : TestFormat_Op<"literal_invalid_b", [{
   `
 }]>;
 // CHECK-NOT: error
 def LiteralValid : TestFormat_Op<"literal_valid", [{
-  `_` `:` `,` `=` `<` `>` `(` `)` `[` `]` `->` `abc$._`
+  `_` `:` `,` `=` `<` `>` `(` `)` `[` `]` ` ` `->` `abc$._`
   attr-dict
 }]>;
 
 //===----------------------------------------------------------------------===//
 // Optional Groups
 //===----------------------------------------------------------------------===//
 
 // CHECK: error: optional groups can only be used as top-level elements

 // CHECK: error: expected '?' after optional group
 def OptionalInvalidK : TestFormat_Op<"optional_invalid_k", [{
   ($arg^)
 }]>, Arguments<(ins Variadic<I64>:$arg)>;
 // CHECK: error: only variables can be used to anchor an optional group
 def OptionalInvalidL : TestFormat_Op<"optional_invalid_l", [{
   (custom<MyDirective>($arg)^)?
 }]>, Arguments<(ins I64:$arg)>;
+// CHECK: error: only variables can be used to anchor an optional group
+def OptionalInvalidM : TestFormat_Op<"optional_invalid_m", [{
+  (` `^)?
+}]>, Arguments<(ins)>;
 
 //===----------------------------------------------------------------------===//
 // Variables
 //===----------------------------------------------------------------------===//
 
 // CHECK: error: expected variable to refer to an argument, region, result, or successor
 def VariableInvalidA : TestFormat_Op<"variable_invalid_a", [{
   $unknown_arg attr-dict

 // CHECK: error: format ambiguity caused by `:` literal found after attribute `attr` which does not have a buildable type
 def VariableInvalidH : TestFormat_Op<"variable_invalid_h", [{
   $attr `:` attr-dict
 }]>, Arguments<(ins ElementsAttr:$attr)>;
 // CHECK: error: format ambiguity caused by `:` literal found after attribute `attr` which does not have a buildable type
 def VariableInvalidI : TestFormat_Op<"variable_invalid_i", [{
   (`foo` $attr^)? `:` attr-dict
 }]>, Arguments<(ins OptionalAttr<ElementsAttr>:$attr)>;
-// CHECK: error: region 'region' is already bound
+// CHECK: error: format ambiguity caused by `:` literal found after attribute `attr` which does not have a buildable type
 def VariableInvalidJ : TestFormat_Op<"variable_invalid_j", [{
+  $attr ` ` `:` attr-dict
+}]>, Arguments<(ins ElementsAttr:$attr)>;
+// CHECK: error: region 'region' is already bound
+def VariableInvalidK : TestFormat_Op<"variable_invalid_k", [{
   $region $region attr-dict
 }]> {
   let regions = (region AnyRegion:$region);
 }
 // CHECK: error: region 'region' is already bound
-def VariableInvalidK : TestFormat_Op<"variable_invalid_K", [{
+def VariableInvalidL : TestFormat_Op<"variable_invalid_l", [{
   regions $region attr-dict
 }]> {
   let regions = (region AnyRegion:$region);
 }
 // CHECK: error: regions can only be used at the top level
-def VariableInvalidL : TestFormat_Op<"variable_invalid_l", [{
+def VariableInvalidM : TestFormat_Op<"variable_invalid_m", [{
   type($region)
 }]> {
   let regions = (region AnyRegion:$region);
 }
 // CHECK: error: region #0, named 'region', not found
-def VariableInvalidM : TestFormat_Op<"variable_invalid_m", [{
+def VariableInvalidN : TestFormat_Op<"variable_invalid_n", [{
   attr-dict
 }]> {
   let regions = (region AnyRegion:$region);
 }
 // CHECK-NOT: error:
 def VariableValidA : TestFormat_Op<"variable_valid_a", [{
   $attr `:` attr-dict
 }]>, Arguments<(ins OptionalAttr<I1Attr>:$attr)>;

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

 // RUN: mlir-opt -allow-unregistered-dialect %s | mlir-opt -allow-unregistered-dialect -verify-diagnostics | FileCheck %s
 
 // CHECK: %[[I64:.*]] =
 %i64 = "foo.op"() : () -> (i64)
 // CHECK: %[[MEMREF:.*]] =
 %memref = "foo.op"() : () -> (memref<1xf64>)
 
-// CHECK: test.format_literal_op keyword_$. -> :, = <> () [] {foo.some_attr}
-test.format_literal_op keyword_$. -> :, = <> () [] {foo.some_attr}
+// CHECK: test.format_literal_op keyword_$. -> :, = <> () [] ( ) {foo.some_attr}
+test.format_literal_op keyword_$. -> :, = <> () [] ( ) {foo.some_attr}
 
 // CHECK: test.format_attr_op 10
 // CHECK-NOT: {attr
 test.format_attr_op 10
 
 // CHECK: test.format_opt_attr_op_a(10)
 // CHECK-NOT: {opt_attr
 test.format_opt_attr_op_a(10)

mlir/tools/mlir-tblgen/OpFormatGen.cpp

enum class Kind {
     ResultsDirective,
     SuccessorsDirective,
     TypeDirective,
     TypeRefDirective,
 
     /// This element is a literal.
     Literal,
 
+    /// This element is printed as a space. It is ignored by the parser.
+    Space,
+
     /// This element is an variable value.
     AttributeVariable,
     OperandVariable,
     RegionVariable,
     ResultVariable,
     SuccessorVariable,
 
     /// This element is an optional element.
bool LiteralElement::isValidLiteral(StringRef value) {
   if (!isalpha(front) && front != '_')
     return false;
   return llvm::all_of(value.drop_front(), [](char c) {
     return isalnum(c) || c == '_' || c == '$' || c == '.';
   });
 }
 
 //===----------------------------------------------------------------------===//
+// SpaceElement
+
+namespace {
+/// This class represents an instance of a space element. It's a literal that
+/// is only printed, but ignored by the parser.
+class SpaceElement : public Element {
+public:
+  SpaceElement() : Element{Kind::Space} {}
+  static bool classof(const Element *element) {
+    return element->getKind() == Kind::Space;
+  }
+};
+} // end anonymous namespace
+
+//===----------------------------------------------------------------------===//
 // OptionalElement
 
 namespace {
 /// This class represents a group of elements that are optionally emitted based
 /// upon an optional variable of the operation.
 class OptionalElement : public Element {
 public:
   OptionalElement(std::vector<std::unique_ptr<Element>> &&elements,
if (auto *optional = dyn_cast<OptionalElement>(element)) {
     body << "  }\n";
 
     /// Literals.
   } else if (LiteralElement *literal = dyn_cast<LiteralElement>(element)) {
     body << "  if (parser.parse";
     genLiteralParser(literal->getLiteral(), body);
     body << ")\n    return ::mlir::failure();\n";
 
+    /// Spaces.
+  } else if (isa<SpaceElement>(element)) {
+    // Nothing to parse.
+
     /// Arguments.
   } else if (auto *attr = dyn_cast<AttributeVariable>(element)) {
     const NamedAttribute *var = attr->getVar();
 
     // Check to see if we can parse this as an enum attribute.
     if (canFormatEnumAttr(var)) {
       const EnumAttr &enumAttr = cast<EnumAttr>(var->attr);
 
static void genLiteralPrinter(StringRef value, OpMethodBody &body,
   auto shouldPrintSpaceBeforeLiteral = [&] {
     if (value.size() != 1 && value != "->")
       return true;
     if (lastWasPunctuation)
       return !StringRef(">)}],").contains(value.front());
     return !StringRef("<>(){}[],").contains(value.front());
   };
   if (shouldEmitSpace && shouldPrintSpaceBeforeLiteral())
-    body << " << \" \"";
+    body << " << ' '";
   body << " << \"" << value << "\";\n";
 
   // Insert a space after certain literals.
   shouldEmitSpace =
       value.size() != 1 || !StringRef("<({[").contains(value.front());
   lastWasPunctuation = !(value.front() == '_' || isalpha(value.front()));
 }
 
-/// Generate the printer for a literal value. `shouldEmitSpace` is true if a
-/// space should be emitted before this element. `lastWasPunctuation` is true if
-/// the previous element was a punctuation literal.
+/// Generate the printer for a space. `shouldEmitSpace` and `lastWasPunctuation`
+/// are set to false.
+static void genSpacePrinter(OpMethodBody &body, bool &shouldEmitSpace,
+                            bool &lastWasPunctuation) {
+  body << "  p << ' ';\n";
+  shouldEmitSpace = false;
+  lastWasPunctuation = false;
+}
+
+/// Generate the printer for a custom directive.
 static void genCustomDirectivePrinter(CustomDirective *customDir,
                                       OpMethodBody &body) {
   body << "  print" << customDir->getName() << "(p";
   for (Element &param : customDir->getArguments()) {
     body << ", ";
     if (auto *attr = dyn_cast<AttributeVariable>(&param)) {
       body << attr->getVar()->name << "Attr()";
 

 
 void OperationFormat::genElementPrinter(Element *element, OpMethodBody &body,
                                         Operator &op, bool &shouldEmitSpace,
                                         bool &lastWasPunctuation) {
   if (LiteralElement *literal = dyn_cast<LiteralElement>(element))
     return genLiteralPrinter(literal->getLiteral(), body, shouldEmitSpace,
                              lastWasPunctuation);
 
+  if (isa<SpaceElement>(element))
+    return genSpacePrinter(body, shouldEmitSpace, lastWasPunctuation);
+
   // Emit an optional group.
   if (OptionalElement *optional = dyn_cast<OptionalElement>(element)) {
     // Emit the check for the presence of the anchor element.
     Element *anchor = optional->getAnchor();
     if (auto *operand = dyn_cast<OperandVariable>(anchor)) {
       const NamedTypeConstraint *var = operand->getVar();
       if (var->isOptional())
         body << "  if (" << var->name << "()) {\n";
if (auto *attrDict = dyn_cast<AttrDictDirective>(element)) {
     genAttrDictPrinter(*this, op, body, attrDict->isWithKeyword());
     lastWasPunctuation = false;
     return;
   }
 
   // Optionally insert a space before the next element. The AttrDict printer
   // already adds a space as necessary.
   if (shouldEmitSpace || !lastWasPunctuation)
-    body << "  p << \" \";\n";
+    body << "  p << ' ';\n";
   lastWasPunctuation = false;
   shouldEmitSpace = true;
 
   if (auto *attr = dyn_cast<AttributeVariable>(element)) {
     const NamedAttribute *var = attr->getVar();
 
     // If we are formatting as an enum, symbolize the attribute as a string.
     if (canFormatEnumAttr(var)) {
       const EnumAttr &enumAttr = cast<EnumAttr>(var->attr);
-      body << "  p << \"\\\"\" << " << enumAttr.getSymbolToStringFnName() << "("
-           << var->name << "()) << \"\\\"\";\n";
+      body << "  p << '\"' << " << enumAttr.getSymbolToStringFnName() << "("
+           << var->name << "()) << '\"';\n";
       return;
     }
 
     // If we are formatting as a symbol name, handle it as a symbol name.
     if (shouldFormatSymbolNameAttr(var)) {
       body << "  p.printSymbolName(" << var->name << "Attr().getValue());\n";
       return;
     }
while (it != e) {
     auto *prevAttr = dyn_cast<AttributeVariable>(element);
     if (!prevAttr || prevAttr->getTypeBuilder())
       continue;
 
     // Check the next iterator within the stack for literal elements.
     for (auto &nextItPair : iteratorStack) {
       ElementsIterT nextIt = nextItPair.first, nextE = nextItPair.second;
       for (; nextIt != nextE; ++nextIt) {
-        // Skip any trailing optional groups or attribute dictionaries.
-        if (isa<AttrDictDirective>(*nextIt) || isa<OptionalElement>(*nextIt))
+        // Skip any trailing spaces, attribute dictionaries, or optional groups.
+        if (isa<SpaceElement>(*nextIt) || isa<AttrDictDirective>(*nextIt) ||
+            isa<OptionalElement>(*nextIt))
           continue;
 
         // We are only interested in `:` literals.
         auto *literal = dyn_cast<LiteralElement>(&*nextIt);
         if (!literal || literal->getLiteral() != ":")
           break;
 
         // TODO: Use the location of the literal element itself.
default:
     llvm_unreachable("unknown directive token");
   }
 }
 
 LogicalResult FormatParser::parseLiteral(std::unique_ptr<Element> &element) {
   Token literalTok = curToken;
   consumeToken();
 
-  // Check that the parsed literal is valid.
   StringRef value = literalTok.getSpelling().drop_front().drop_back();
+
+  // The parsed literal is a space.
+  if (value.size() == 1 && value.front() == ' ') {
+    element = std::make_unique<SpaceElement>();
+    return ::mlir::success();
+  }
+
+  // Check that the parsed literal is valid.
   if (!LiteralElement::isValidLiteral(value))
     return emitError(literalTok.getLoc(), "expected valid literal");
 
   element = std::make_unique<LiteralElement>(value);
   return ::mlir::success();
 }
 
 LogicalResult FormatParser::parseOptional(std::unique_ptr<Element> &element,
return TypeSwitch<Element *, LogicalResult>(childElements.back().get())
         seenVariables.insert(ele->getVar());
         return ::mlir::success();
       })
       .Case<RegionVariable>([&](RegionVariable *) {
         // TODO: When ODS has proper support for marking "optional" regions, add
         // a check here.
         return ::mlir::success();
       })
-      // Literals, custom directives, and type directives may be used,
+      // Literals, spaces, custom directives, and type directives may be used,
       // but they can't anchor the group.
-      .Case<LiteralElement, CustomDirective, FunctionalTypeDirective,
-            OptionalElement, TypeRefDirective, TypeDirective>([&](Element *) {
+      .Case<LiteralElement, SpaceElement, CustomDirective,
+            FunctionalTypeDirective, OptionalElement, TypeRefDirective,
+            TypeDirective>([&](Element *) {
         if (isAnchor)
           return emitError(childLoc, "only variables can be used to anchor "
                                      "an optional group");
         return ::mlir::success();
       })
       .Default([&](Element *) {
         return emitError(childLoc, "only literals, types, and variables can be "
                                    "used within an optional group");