[mlir][DeclarativeParser] Add support for formatting enum attributes in the string form.

[rriddle]

Summary

This revision adds support to the declarative parser for formatting enum attributes in the symbolized form. It uses this new functionality to port several of the SPIRV parsers over to the declarative form.

[antiagainst]

Nice! Thanks River for adding support for this!

mlir/tools/mlir-tblgen/OpFormatGen.cpp

301

Do we want to specify none type here? It should parse as a string attr for start and we later reply on that for symbolize the string.

[rriddle]

mlir/tools/mlir-tblgen/OpFormatGen.cpp

301

Without specifying a type, the parser of StringAttr will check for an optional colon type which isn't a desirable. Passing NoneType makes sure that we only parse the string value.

[flaub]

This is great! Would it be possible to parse keywords into enum attrs as well?

Actually, what's the general feeling about using keywords vs strings for enum values? I think I prefer keywords a little more, it's less noisy and its distinguished from an actual string in the textual representation. Also, it seems like LLVM IR has 'keywords' (they aren't wrapped in quotes) for many enums.

[rriddle]

In D74525#1875374, @flaub wrote:

This is great! Would it be possible to parse keywords into enum attrs as well?

Actually, what's the general feeling about using keywords vs strings for enum values? I think I prefer keywords a little more, it's less noisy and its distinguished from an actual string in the textual representation. Also, it seems like LLVM IR has 'keywords' (they aren't wrapped in quotes) for many enums.

I think we could, but we would need additional checks given that only a subset of enums can actually be represented with a keyword. BitEnums would be completely out, but that is to be expected. For the others we would need to check dynamically if the case value can be parsed in as a keyword. I say dynamically instead of statically because it would be really awful if you added a new enum case that wasn't conforming and then had to update all of your existing tests.

mlir/include/mlir/Dialect/SPIRV/SPIRVNonUniformOps.td

def SPV_GroupNonUniformBallotOp : SPV_Op<"GroupNonUniformBallot", []> {
   let arguments = (ins
     SPV_ScopeAttr:$execution_scope,
     SPV_Bool:$predicate
   );
 
   let results = (outs
     SPV_IntVec4:$result
   );
+
+  let assemblyFormat = [{
+    $execution_scope $predicate attr-dict `:` type($result)
+  }];
 }
 
 // -----
 
 def SPV_GroupNonUniformElectOp : SPV_Op<"GroupNonUniformElect", []> {
   let summary = [{
     Result is true only in the active invocation with the lowest id in the
     group, otherwise result is false.
def SPV_GroupNonUniformElectOp : SPV_Op<"GroupNonUniformElect", []> {
 
   let results = (outs
     SPV_Bool:$result
   );
 
   let builders = [
     OpBuilder<[{Builder *builder, OperationState &state, spirv::Scope}]>
   ];
+
+  let assemblyFormat = "$execution_scope attr-dict `:` type($result)";
 }
 
 // -----
 
 def SPV_GroupNonUniformFAddOp :
     SPV_GroupNonUniformArithmeticOp<"GroupNonUniformFAdd", SPV_Float, []> {
   let summary = [{
     A floating point add group operation of all Value operands contributed

mlir/include/mlir/Dialect/SPIRV/SPIRVOps.td

let arguments = (ins
     SPV_MemorySemanticsAttr:$memory_semantics
   );
 
   let results = (outs);
 
   let verifier = [{ return verifyMemorySemantics(*this); }];
 
   let autogenSerialization = 0;
+
+  let assemblyFormat = [{
+    $execution_scope `,` $memory_scope `,` $memory_semantics attr-dict
+  }];
 }
 
 // -----
 
 def SPV_ExecutionModeOp : SPV_Op<"ExecutionMode", [InModuleScope]> {
   let summary = "Declare an execution mode for an entry point.";
 
   let description = [{
let arguments = (ins
     SPV_MemorySemanticsAttr:$memory_semantics
   );
 
   let results = (outs);
 
   let verifier = [{ return verifyMemorySemantics(*this); }];
 
   let autogenSerialization = 0;
+
+  let assemblyFormat = "$memory_scope `,` $memory_semantics attr-dict";
 }
 
 // -----
 
 def SPV_StoreOp : SPV_Op<"Store", []> {
   let summary = "Store through a pointer.";
 
   let description = [{

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp

if (auto intType = type.dyn_cast<IntegerType>()) {
     return builder->create<spirv::ConstantOp>(
         loc, type, builder->getIntegerAttr(type, APInt(width, 1)));
   }
 
   llvm_unreachable("unimplemented types for ConstantOp::getOne()");
 }
 
 //===----------------------------------------------------------------------===//
-// spv.ControlBarrier
-//===----------------------------------------------------------------------===//
-
-static ParseResult parseControlBarrierOp(OpAsmParser &parser,
-                                         OperationState &state) {
-  spirv::Scope executionScope;
-  spirv::Scope memoryScope;
-  spirv::MemorySemantics memorySemantics;
-
-  return failure(
-      parseEnumAttribute(executionScope, parser, state,
-                         kExecutionScopeAttrName) ||
-      parser.parseComma() ||
-      parseEnumAttribute(memoryScope, parser, state, kMemoryScopeAttrName) ||
-      parser.parseComma() ||
-      parseEnumAttribute(memorySemantics, parser, state));
-}
-
-static void print(spirv::ControlBarrierOp op, OpAsmPrinter &printer) {
-  printer << spirv::ControlBarrierOp::getOperationName() << " \""
-          << stringifyScope(op.execution_scope()) << "\", \""
-          << stringifyScope(op.memory_scope()) << "\", \""
-          << stringifyMemorySemantics(op.memory_semantics()) << "\"";
-}
-
-//===----------------------------------------------------------------------===//
 // spv.EntryPoint
 //===----------------------------------------------------------------------===//
 
 void spirv::EntryPointOp::build(Builder *builder, OperationState &state,
                                 spirv::ExecutionModel executionModel,
                                 spirv::FuncOp function,
                                 ArrayRef<Attribute> interfaceVars) {
   build(builder, state,
static LogicalResult verify(spirv::GlobalVariableOp varOp) {
 
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // spv.GroupNonUniformBallotOp
 //===----------------------------------------------------------------------===//
 
-static ParseResult parseGroupNonUniformBallotOp(OpAsmParser &parser,
-                                                OperationState &state) {
-  spirv::Scope executionScope;
-  OpAsmParser::OperandType operandInfo;
-  Type resultType;
-  IntegerType i1Type = parser.getBuilder().getI1Type();
-  if (parseEnumAttribute(executionScope, parser, state,
-                         kExecutionScopeAttrName) ||
-      parser.parseOperand(operandInfo) || parser.parseColonType(resultType) ||
-      parser.resolveOperand(operandInfo, i1Type, state.operands))
-    return failure();
-
-  return parser.addTypeToList(resultType, state.types);
-}
-
-static void print(spirv::GroupNonUniformBallotOp ballotOp,
-                  OpAsmPrinter &printer) {
-  printer << spirv::GroupNonUniformBallotOp::getOperationName() << " \""
-          << stringifyScope(ballotOp.execution_scope()) << "\" "
-          << ballotOp.predicate() << " : " << ballotOp.getType();
-}
-
 static LogicalResult verify(spirv::GroupNonUniformBallotOp ballotOp) {
   // TODO(antiagainst): check the result integer type's signedness bit is 0.
 
   spirv::Scope scope = ballotOp.execution_scope();
   if (scope != spirv::Scope::Workgroup && scope != spirv::Scope::Subgroup)
     return ballotOp.emitOpError(
         "execution scope must be 'Workgroup' or 'Subgroup'");
 
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // spv.GroupNonUniformElectOp
 //===----------------------------------------------------------------------===//
 
 void spirv::GroupNonUniformElectOp::build(Builder *builder,
                                           OperationState &state,
                                           spirv::Scope scope) {
   build(builder, state, builder->getI1Type(), scope);
 }
 
-static ParseResult parseGroupNonUniformElectOp(OpAsmParser &parser,
-                                               OperationState &state) {
-  spirv::Scope executionScope;
-  Type resultType;
-  if (parseEnumAttribute(executionScope, parser, state,
-                         kExecutionScopeAttrName) ||
-      parser.parseColonType(resultType))
-    return failure();
-
-  return parser.addTypeToList(resultType, state.types);
-}
-
-static void print(spirv::GroupNonUniformElectOp groupOp,
-                  OpAsmPrinter &printer) {
-  printer << spirv::GroupNonUniformElectOp::getOperationName() << " \""
-          << stringifyScope(groupOp.execution_scope())
-          << "\" : " << groupOp.getType();
-}
-
 static LogicalResult verify(spirv::GroupNonUniformElectOp groupOp) {
   spirv::Scope scope = groupOp.execution_scope();
   if (scope != spirv::Scope::Workgroup && scope != spirv::Scope::Subgroup)
     return groupOp.emitOpError(
         "execution scope must be 'Workgroup' or 'Subgroup'");
 
   return success();
 }
 
-
-
 //===----------------------------------------------------------------------===//
 // spv.IAdd
 //===----------------------------------------------------------------------===//
 
 OpFoldResult spirv::IAddOp::fold(ArrayRef<Attribute> operands) {
   assert(operands.size() == 2 && "spv.IAdd expects two operands");
   // x + 0 = x
   if (matchPattern(operand2(), m_Zero()))
static LogicalResult verify(spirv::MergeOp mergeOp) {
   Block &parentLastBlock = mergeOp.getParentRegion()->back();
   if (mergeOp.getOperation() != parentLastBlock.getTerminator())
     return mergeOp.emitOpError(
         "can only be used in the last block of 'spv.selection' or 'spv.loop'");
   return success();
 }
 
 //===----------------------------------------------------------------------===//
-// spv.MemoryBarrier
-//===----------------------------------------------------------------------===//
-
-static ParseResult parseMemoryBarrierOp(OpAsmParser &parser,
-                                        OperationState &state) {
-  spirv::Scope memoryScope;
-  spirv::MemorySemantics memorySemantics;
-
-  return failure(
-      parseEnumAttribute(memoryScope, parser, state, kMemoryScopeAttrName) ||
-      parser.parseComma() ||
-      parseEnumAttribute(memorySemantics, parser, state));
-}
-
-static void print(spirv::MemoryBarrierOp op, OpAsmPrinter &printer) {
-  printer << spirv::MemoryBarrierOp::getOperationName() << " \""
-          << stringifyScope(op.memory_scope()) << "\", \""
-          << stringifyMemorySemantics(op.memory_semantics()) << "\"";
-}
-
-//===----------------------------------------------------------------------===//
 // spv.module
 //===----------------------------------------------------------------------===//
 
 void spirv::ModuleOp::build(Builder *builder, OperationState &state) {
   ensureTerminator(*state.addRegion(), *builder, state.location);
 }
 
 // TODO(ravishankarm): This is only here for resolving some dependency outside

mlir/test/Dialect/SPIRV/ops.mlir

func @control_barrier_0() -> () {
   // CHECK:  spv.ControlBarrier "Workgroup", "Device", "Acquire|UniformMemory"
   spv.ControlBarrier "Workgroup", "Device", "Acquire|UniformMemory"
   return
 }
 
 // -----
 
 func @control_barrier_1() -> () {
-  // expected-error @+1 {{invalid scope attribute specification: "Something"}}
+  // expected-error @+1 {{invalid execution_scope attribute specification: "Something"}}
   spv.ControlBarrier "Something", "Device", "Acquire|UniformMemory"
   return
 }
 
 // -----
 
 //===----------------------------------------------------------------------===//
 // spv.ConvertFToS

mlir/tools/mlir-tblgen/OpFormatGen.cpp

struct OperationFormat {
   /// The index of the buildable type, if valid, for every operand and result.
   std::vector<TypeResolution> operandTypes, resultTypes;
 };
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // Parser Gen
 
+/// Returns if we can format the given attribute as an EnumAttr in the parser
+/// format.
+static bool canFormatEnumAttr(const NamedAttribute *attr) {
+  const EnumAttr *enumAttr = dyn_cast<EnumAttr>(&attr->attr);
+  if (!enumAttr)
+    return false;
+
+  // The attribute must have a valid underlying type and a constant builder.
+  return !enumAttr->getUnderlyingType().empty() &&
+         !enumAttr->getConstBuilderTemplate().empty();
+}
+
 /// The code snippet used to generate a parser call for an attribute.
 ///
 /// {0}: The storage type of the attribute.
 /// {1}: The name of the attribute.
 /// {2}: The type for the attribute.
 const char *const attrParserCode = R"(
   {0} {1}Attr;
   if (parser.parseAttribute({1}Attr{2}, "{1}", result.attributes))
     return failure();
 )";
 
+/// The code snippet used to generate a parser call for an enum attribute.
+///
+/// {0}: The name of the attribute.
+/// {1}: The c++ namespace for the enum symbolize functions.
+/// {2}: The function to symbolize a string of the enum.
+/// {3}: The constant builder call to create an attribute of the enum type.
+const char *const enumAttrParserCode = R"(
+  {
+    StringAttr attrVal;
+    SmallVector<NamedAttribute, 1> attrStorage;
+    auto loc = parser.getCurrentLocation();
+    if (parser.parseAttribute(attrVal, parser.getBuilder().getNoneType(),
+                              "{0}", attrStorage))
+      return failure();
+
+    auto attrOptional = {1}::{2}(attrVal.getValue());
+    if (!attrOptional)
+      return parser.emitError(loc, "invalid ")
+             << "{0} attribute specification: " << attrVal;
+
+    result.addAttribute("{0}", {3});
+  }
+)";
+
 /// The code snippet used to generate a parser call for an operand.
 ///
 /// {0}: The name of the operand.
 const char *const variadicOperandParserCode = R"(
   llvm::SMLoc {0}OperandsLoc = parser.getCurrentLocation();
   (void){0}OperandsLoc;
   SmallVector<OpAsmParser::OperandType, 4> {0}Operands;
   if (parser.parseOperandList({0}Operands))
for (auto &element : elements) {
     /// Literals.
     if (LiteralElement *literal = dyn_cast<LiteralElement>(element.get())) {
       genLiteralParser(literal->getLiteral(), body);
 
       /// Arguments.
     } else if (auto *attr = dyn_cast<AttributeVariable>(element.get())) {
       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);
+
+        // Generate the code for building an attribute for this enum.
+        std::string attrBuilderStr;
+        {
+          llvm::raw_string_ostream os(attrBuilderStr);
+          os << tgfmt(enumAttr.getConstBuilderTemplate(), &attrTypeCtx,
+                      "attrOptional.getValue()");
+        }
+
+        body << formatv(enumAttrParserCode, var->name,
+                        enumAttr.getCppNamespace(),
+                        enumAttr.getStringToSymbolFnName(), attrBuilderStr);
+        continue;
+      }
+
       // If this attribute has a buildable type, use that when parsing the
       // attribute.
       std::string attrTypeStr;
       if (Optional<Type> attrType = var->attr.getValueType()) {
         if (Optional<StringRef> typeBuilder = attrType->getBuilderCall()) {
           llvm::raw_string_ostream os(attrTypeStr);
           os << ", " << tgfmt(*typeBuilder, &attrTypeCtx);
         }
for (auto &element : elements) {
     if (shouldEmitSpace || !lastWasPunctuation)
       body << "  p << \" \";\n";
     lastWasPunctuation = false;
     shouldEmitSpace = true;
 
     if (auto *attr = dyn_cast<AttributeVariable>(element.get())) {
       const NamedAttribute *var = attr->getVar();
 
-      // Elide the attribute type if it is buildable..
+      // If we are formatting as a enum, symbolize the attribute as a string.
+      if (canFormatEnumAttr(var)) {
+        const EnumAttr &enumAttr = cast<EnumAttr>(var->attr);
+        body << "  p << \"\\\"\" << " << enumAttr.getSymbolToStringFnName()
+             << "(" << var->name << "()) << \"\\\"\";\n";
+        continue;
+      }
+
+      // Elide the attribute type if it is buildable.
       Optional<Type> attrType = var->attr.getValueType();
       if (attrType && attrType->getBuilderCall())
         body << "  p.printAttributeWithoutType(" << var->name << "Attr());\n";
       else
         body << "  p.printAttribute(" << var->name << "Attr());\n";
     } else if (auto *operand = dyn_cast<OperandVariable>(element.get())) {
       body << "  p << " << operand->getVar()->name << "();\n";
     } else if (isa<OperandsDirective>(element.get())) {