[mlir] Add support for non-identifier attribute names.

[rriddle]

Summary

In some situations the name of the attribute is not representable as a bare-identifier, this revision adds support for those cases by formatting the name as a string instead. This has the added benefit of removing the identifier regex from the verifier.

[silvas]

Nice improvement! Thanks for the quick turnaround :)

mlir/lib/IR/AsmPrinter.cpp

1622

nit: '"' for consistency with below

[rriddle]

Resolve comments

mlir/docs/LangRef.md

 Syntax:
 
 ```
 attribute-dict ::= `{` `}`
                  | `{` attribute-entry (`,` attribute-entry)* `}`
 attribute-entry ::= dialect-attribute-entry | dependent-attribute-entry
 dialect-attribute-entry ::= dialect-namespace `.` bare-id `=` attribute-value
 dependent-attribute-entry ::= dependent-attribute-name `=` attribute-value
-dependent-attribute-name ::= (letter|[_]) (letter|digit|[_$])*
+dependent-attribute-name ::= ((letter|[_]) (letter|digit|[_$])*)
+                           | string-literal
 ```
 
 Attributes are the mechanism for specifying constant data on operations in
 places where a variable is never allowed - e.g. the index of a
 [`dim` operation](Dialects/Standard.md#dim-operation), or the stride of a
 convolution. They consist of a name and a concrete attribute value. The set of
 expected attributes, their structure, and their interpretation are all
 contextually dependent on what they are attached to.

mlir/lib/Analysis/Verifier.cpp

 #include "llvm/Support/Regex.h"
 
 using namespace mlir;
 
 namespace {
 /// This class encapsulates all the state used to verify an operation region.
 class OperationVerifier {
 public:
-  explicit OperationVerifier(MLIRContext *ctx)
-      : ctx(ctx), identifierRegex("^[a-zA-Z_][a-zA-Z_0-9\\.\\$]*$") {}
+  explicit OperationVerifier(MLIRContext *ctx) : ctx(ctx) {}
 
   /// Verify the given operation.
   LogicalResult verify(Operation &op);
 
   /// Returns the registered dialect for a dialect-specific attribute.
   Dialect *getDialectForAttribute(const NamedAttribute &attr) {
     assert(attr.first.strref().contains('.') && "expected dialect attribute");
     auto dialectNamePair = attr.first.strref().split('.');
     return ctx->getRegisteredDialect(dialectNamePair.first);
   }
 
-  /// Returns if the given string is valid to use as an identifier name.
-  bool isValidName(StringRef name) { return identifierRegex.match(name); }
-
 private:
   /// Verify the given potentially nested region or block.
   LogicalResult verifyRegion(Region &region);
   LogicalResult verifyBlock(Block &block);
   LogicalResult verifyOperation(Operation &op);
 
   /// Verify the dominance within the given IR unit.
   LogicalResult verifyDominance(Region &region);
private:
   }
 
   /// The current context for the verifier.
   MLIRContext *ctx;
 
   /// Dominance information for this operation, when checking dominance.
   DominanceInfo *domInfo = nullptr;
 
-  /// Regex checker for attribute names.
-  llvm::Regex identifierRegex;
-
   /// Mapping between dialect namespace and if that dialect supports
   /// unregistered operations.
   llvm::StringMap<bool> dialectAllowsUnknownOps;
 };
 } // end anonymous namespace
 
 /// Verify the given operation.
 LogicalResult OperationVerifier::verify(Operation &op) {

 LogicalResult OperationVerifier::verifyOperation(Operation &op) {
   // Check that operands are non-nil and structurally ok.
   for (auto operand : op.getOperands())
     if (!operand)
       return op.emitError("null operand found");
 
   /// Verify that all of the attributes are okay.
   for (auto attr : op.getAttrs()) {
-    if (!identifierRegex.match(attr.first))
-      return op.emitError("invalid attribute name '") << attr.first << "'";
-
     // Check for any optional dialect specific attributes.
     if (!attr.first.strref().contains('.'))
       continue;
     if (auto *dialect = getDialectForAttribute(attr))
       if (failed(dialect->verifyOperationAttribute(&op, attr)))
         return failure();
   }
 

mlir/lib/IR/AsmPrinter.cpp

printAffineExpr(AffineExpr expr,
                   function_ref<void(unsigned, bool)> printValueName = nullptr);
   void printAffineConstraint(AffineExpr expr, bool isEq);
   void printIntegerSet(IntegerSet set);
 
 protected:
   void printOptionalAttrDict(ArrayRef<NamedAttribute> attrs,
                              ArrayRef<StringRef> elidedAttrs = {},
                              bool withKeyword = false);
+  void printNamedAttribute(NamedAttribute attr);
   void printTrailingLocation(Location loc);
   void printLocationInternal(LocationAttr loc, bool pretty = false);
 
   /// Print a dense elements attribute. If 'allowHex' is true, a hex string is
   /// used instead of individual elements when the elements attr is large.
   void printDenseElementsAttr(DenseElementsAttr attr, bool allowHex);
 
   void printDialectAttribute(Attribute attr);
void ModulePrinter::printAttribute(Attribute attr,
   case StandardAttributes::Bool:
     os << (attr.cast<BoolAttr>().getValue() ? "true" : "false");
 
     // BoolAttr always elides the type.
     return;
   case StandardAttributes::Dictionary:
     os << '{';
     interleaveComma(attr.cast<DictionaryAttr>().getValue(),
-                    [&](NamedAttribute attr) {
-                      os << attr.first;
-
-                      // The value of a UnitAttr is elided within a dictionary.
-                      if (attr.second.isa<UnitAttr>())
-                        return;
-
-                      os << " = ";
-                      printAttribute(attr.second);
-                    });
+                    [&](NamedAttribute attr) { printNamedAttribute(attr); });
     os << '}';
     break;
   case StandardAttributes::Integer: {
     auto intAttr = attr.cast<IntegerAttr>();
     // Print all signed/signless integer attributes as signed unless i1.
     bool isSigned =
         attrType.isIndex() || (!attrType.isUnsignedInteger() &&
                                attrType.getIntOrFloatBitWidth() != 1);
if (filteredAttrs.empty())
     return;
 
   // Print the 'attributes' keyword if necessary.
   if (withKeyword)
     os << " attributes";
 
   // Otherwise, print them all out in braces.
   os << " {";
-  interleaveComma(filteredAttrs, [&](NamedAttribute attr) {
+  interleaveComma(filteredAttrs,
+                  [&](NamedAttribute attr) { printNamedAttribute(attr); });
+  os << '}';
+}
+
+void ModulePrinter::printNamedAttribute(NamedAttribute attr) {
+  if (isBareIdentifier(attr.first)) {
     os << attr.first;
+  } else {
+    os << '"';
+    printEscapedString(attr.first.strref(), os);
+    os << '"';
+  }
 
-    // Pretty printing elides the attribute value for unit attributes.
-    if (attr.second.isa<UnitAttr>())
-      return;
 
-    os << " = ";
-    printAttribute(attr.second);
-  });
-  os << '}';
 }
 
 //===----------------------------------------------------------------------===//
 // CustomDialectAsmPrinter
 //===----------------------------------------------------------------------===//
 
 namespace {
 /// This class provides the main specialization of the DialectAsmPrinter that is

mlir/lib/Parser/Parser.cpp

default:
     return nullptr;
   }
 }
 
 /// Attribute dictionary.
 ///
 ///   attribute-dict ::= `{` `}`
 ///                    | `{` attribute-entry (`,` attribute-entry)* `}`
-///   attribute-entry ::= bare-id `=` attribute-value
+///   attribute-entry ::= (bare-id | string-literal) `=` attribute-value
 ///
 ParseResult
 Parser::parseAttributeDict(SmallVectorImpl<NamedAttribute> &attributes) {
   if (parseToken(Token::l_brace, "expected '{' in attribute dictionary"))
     return failure();
 
   auto parseElt = [&]() -> ParseResult {
-    // We allow keywords as attribute names.
-    if (getToken().isNot(Token::bare_identifier, Token::inttype) &&
-        !getToken().isKeyword())
+    // The name of an attribute can either be a bare identifier, or a string.
+    Optional<Identifier> nameId;
+    if (getToken().is(Token::string))
+      nameId = builder.getIdentifier(getToken().getStringValue());
+    else if (getToken().isAny(Token::bare_identifier, Token::inttype) ||
+             getToken().isKeyword())
+      nameId = builder.getIdentifier(getTokenSpelling());
+    else
       return emitError("expected attribute name");
-    Identifier nameId = builder.getIdentifier(getTokenSpelling());
     consumeToken();
 
     // Try to parse the '=' for the attribute value.
     if (!consumeIf(Token::equal)) {
       // If there is no '=', we treat this as a unit attribute.
-      attributes.push_back({nameId, builder.getUnitAttr()});
+      attributes.push_back({*nameId, builder.getUnitAttr()});
       return success();
     }
 
     auto attr = parseAttribute();
     if (!attr)
       return failure();
 
-    attributes.push_back({nameId, attr});
+    attributes.push_back({*nameId, attr});
     return success();
   };
 
   if (parseCommaSeparatedListUntil(Token::r_brace, parseElt))
     return failure();
 
   return success();
 }

mlir/test/IR/parser.mlir

 
 // CHECK-LABEL: func @"\22_string_symbol_reference\22"
 func @"\"_string_symbol_reference\""() {
   // CHECK: ref = @"\22_string_symbol_reference\22"
   "foo.symbol_reference"() {ref = @"\"_string_symbol_reference\""} : () -> ()
   return
 }
 
+// CHECK-LABEL: func @string_attr_name
+// CHECK-SAME: {"0 . 0", nested = {"0 . 0"}}
+func @string_attr_name() attributes {"0 . 0", nested = {"0 . 0"}}
+
 // CHECK-LABEL: func @nested_reference
 // CHECK: ref = @some_symbol::@some_nested_symbol
 func @nested_reference() attributes {test.ref = @some_symbol::@some_nested_symbol }
 
 // CHECK-LABEL: func @custom_asm_names
 func @custom_asm_names() -> (i32, i32, i32, i32, i32, i32, i32) {
   // CHECK: %[[FIRST:first.*]], %[[MIDDLE:middle_results.*]]:2, %[[LAST:[0-9]+]]
   %0, %1:2, %2 = "test.asm_interface_op"() : () -> (i32, i32, i32, i32)