[MLIR] LLVM dialect: modernize and cleanups

[flaub]

Summary

Modernize some of the existing custom parsing code in the LLVM dialect.
While this reduces some boilerplate code, it also reduces the precision
of the diagnostic error messges.

[flaub]

I'm not sure this is a net-win because of the less effective error messages, but I thought I'd submit this and see what happens.

[rriddle]

mlir/test/Dialect/LLVMIR/invalid.mlir

203

Random note, it'd be nice to fix this error message to be a bit more helpful(not in this revision). We could likely use llvm::getTypeName<AttrT> inside of the parser to at least give the expected attribute class name.

[merge_guards_bot]

Unit tests: pass. 61987 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[rriddle]

In D72967#1827793, @flaub wrote:

I'm not sure this is a net-win because of the less effective error messages, but I thought I'd submit this and see what happens.

See my other comment. I think we can easily fixup the error messages to be more effective, by:
a) Using the attribute name in the message
b) Using llvm::getTypeName<> to print the c++ type of the expected attribute

[flaub]

• clang-format

[merge_guards_bot]

Unit tests: pass. 61987 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

 // <operation> ::= `llvm.icmp` string-literal ssa-use `,` ssa-use
 //                 attribute-dict? `:` type
 // <operation> ::= `llvm.fcmp` string-literal ssa-use `,` ssa-use
 //                 attribute-dict? `:` type
 template <typename CmpPredicateType>
 static ParseResult parseCmpOp(OpAsmParser &parser, OperationState &result) {
   Builder &builder = parser.getBuilder();
 
-  Attribute predicate;
-  SmallVector<NamedAttribute, 4> attrs;
+  StringAttr predicateAttr;
   OpAsmParser::OperandType lhs, rhs;
   Type type;
   llvm::SMLoc predicateLoc, trailingTypeLoc;
   if (parser.getCurrentLocation(&predicateLoc) ||
-      parser.parseAttribute(predicate, "predicate", attrs) ||
+      parser.parseAttribute(predicateAttr, "predicate", result.attributes) ||
       parser.parseOperand(lhs) || parser.parseComma() ||
-      parser.parseOperand(rhs) || parser.parseOptionalAttrDict(attrs) ||
-      parser.parseColon() || parser.getCurrentLocation(&trailingTypeLoc) ||
-      parser.parseType(type) ||
+      parser.parseOperand(rhs) ||
+      parser.parseOptionalAttrDict(result.attributes) || parser.parseColon() ||
+      parser.getCurrentLocation(&trailingTypeLoc) || parser.parseType(type) ||
       parser.resolveOperand(lhs, type, result.operands) ||
       parser.resolveOperand(rhs, type, result.operands))
     return failure();
 
   // Replace the string attribute `predicate` with an integer attribute.
-  auto predicateStr = predicate.dyn_cast<StringAttr>();
-  if (!predicateStr)
-    return parser.emitError(predicateLoc,
-                            "expected 'predicate' attribute of string type");
-
   int64_t predicateValue = 0;
   if (std::is_same<CmpPredicateType, ICmpPredicate>()) {
     Optional<ICmpPredicate> predicate =
-        symbolizeICmpPredicate(predicateStr.getValue());
+        symbolizeICmpPredicate(predicateAttr.getValue());
     if (!predicate)
       return parser.emitError(predicateLoc)
-             << "'" << predicateStr.getValue()
+             << "'" << predicateAttr.getValue()
              << "' is an incorrect value of the 'predicate' attribute";
     predicateValue = static_cast<int64_t>(predicate.getValue());
   } else {
     Optional<FCmpPredicate> predicate =
-        symbolizeFCmpPredicate(predicateStr.getValue());
+        symbolizeFCmpPredicate(predicateAttr.getValue());
     if (!predicate)
       return parser.emitError(predicateLoc)
-             << "'" << predicateStr.getValue()
+             << "'" << predicateAttr.getValue()
              << "' is an incorrect value of the 'predicate' attribute";
     predicateValue = static_cast<int64_t>(predicate.getValue());
   }
 
-  attrs[0].second = parser.getBuilder().getI64IntegerAttr(predicateValue);
+  result.attributes[0].second =
+      parser.getBuilder().getI64IntegerAttr(predicateValue);
 
   // The result type is either i1 or a vector type <? x i1> if the inputs are
   // vectors.
   auto *dialect = builder.getContext()->getRegisteredDialect<LLVMDialect>();
   auto resultType = LLVMType::getInt1Ty(dialect);
   auto argType = type.dyn_cast<LLVM::LLVMType>();
   if (!argType)
     return parser.emitError(trailingTypeLoc, "expected LLVM IR dialect type");
   if (argType.getUnderlyingType()->isVectorTy())
     resultType = LLVMType::getVectorTy(
         resultType, argType.getUnderlyingType()->getVectorNumElements());
 
-  result.attributes = attrs;
   result.addTypes({resultType});
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Printing/parsing for LLVM::AllocaOp.
 //===----------------------------------------------------------------------===//
 
static void printAllocaOp(OpAsmPrinter &p, AllocaOp &op) {
   else
     p.printOptionalAttrDict(op.getAttrs(), {"alignment"});
   p << " : " << funcTy;
 }
 
 // <operation> ::= `llvm.alloca` ssa-use `x` type attribute-dict?
 //                 `:` type `,` type
 static ParseResult parseAllocaOp(OpAsmParser &parser, OperationState &result) {
-  SmallVector<NamedAttribute, 4> attrs;
   OpAsmParser::OperandType arraySize;
   Type type, elemType;
   llvm::SMLoc trailingTypeLoc;
   if (parser.parseOperand(arraySize) || parser.parseKeyword("x") ||
-      parser.parseType(elemType) || parser.parseOptionalAttrDict(attrs) ||
-      parser.parseColon() || parser.getCurrentLocation(&trailingTypeLoc) ||
-      parser.parseType(type))
+      parser.parseType(elemType) ||
+      parser.parseOptionalAttrDict(result.attributes) || parser.parseColon() ||
+      parser.getCurrentLocation(&trailingTypeLoc) || parser.parseType(type))
     return failure();
 
   // Extract the result type from the trailing function type.
   auto funcType = type.dyn_cast<FunctionType>();
   if (!funcType || funcType.getNumInputs() != 1 ||
       funcType.getNumResults() != 1)
     return parser.emitError(
         trailingTypeLoc,
         "expected trailing function type with one argument and one result");
 
   if (parser.resolveOperand(arraySize, funcType.getInput(0), result.operands))
     return failure();
 
-  result.attributes = attrs;
   result.addTypes({funcType.getResult(0)});
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Printing/parsing for LLVM::GEPOp.
 //===----------------------------------------------------------------------===//
 
 static void printGEPOp(OpAsmPrinter &p, GEPOp &op) {
   SmallVector<Type, 8> types(op.getOperandTypes());
   auto funcTy = FunctionType::get(types, op.getType(), op.getContext());
 
   p << op.getOperationName() << ' ' << op.base() << '['
     << op.getOperands().drop_front() << ']';
   p.printOptionalAttrDict(op.getAttrs());
   p << " : " << funcTy;
 }
 
 // <operation> ::= `llvm.getelementptr` ssa-use `[` ssa-use-list `]`
 //                 attribute-dict? `:` type
 static ParseResult parseGEPOp(OpAsmParser &parser, OperationState &result) {
-  SmallVector<NamedAttribute, 4> attrs;
   OpAsmParser::OperandType base;
   SmallVector<OpAsmParser::OperandType, 8> indices;
   Type type;
   llvm::SMLoc trailingTypeLoc;
   if (parser.parseOperand(base) ||
       parser.parseOperandList(indices, OpAsmParser::Delimiter::Square) ||
-      parser.parseOptionalAttrDict(attrs) || parser.parseColon() ||
+      parser.parseOptionalAttrDict(result.attributes) || parser.parseColon() ||
       parser.getCurrentLocation(&trailingTypeLoc) || parser.parseType(type))
     return failure();
 
   // Deconstruct the trailing function type to extract the types of the base
   // pointer and result (same type) and the types of the indices.
   auto funcType = type.dyn_cast<FunctionType>();
   if (!funcType || funcType.getNumResults() != 1 ||
       funcType.getNumInputs() == 0)
     return parser.emitError(trailingTypeLoc,
                             "expected trailing function type with at least "
                             "one argument and one result");
 
   if (parser.resolveOperand(base, funcType.getInput(0), result.operands) ||
       parser.resolveOperands(indices, funcType.getInputs().drop_front(),
                              parser.getNameLoc(), result.operands))
     return failure();
 
-  result.attributes = attrs;
   result.addTypes(funcType.getResults());
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Printing/parsing for LLVM::LoadOp.
 //===----------------------------------------------------------------------===//
 
static Type getLoadStoreElementType(OpAsmParser &parser, Type type,
   if (!llvmTy.getUnderlyingType()->isPointerTy())
     return parser.emitError(trailingTypeLoc, "expected LLVM pointer type"),
            nullptr;
   return llvmTy.getPointerElementTy();
 }
 
 // <operation> ::= `llvm.load` ssa-use attribute-dict? `:` type
 static ParseResult parseLoadOp(OpAsmParser &parser, OperationState &result) {
-  SmallVector<NamedAttribute, 4> attrs;
   OpAsmParser::OperandType addr;
   Type type;
   llvm::SMLoc trailingTypeLoc;
 
-  if (parser.parseOperand(addr) || parser.parseOptionalAttrDict(attrs) ||
-      parser.parseColon() || parser.getCurrentLocation(&trailingTypeLoc) ||
-      parser.parseType(type) ||
+  if (parser.parseOperand(addr) ||
+      parser.parseOptionalAttrDict(result.attributes) || parser.parseColon() ||
+      parser.getCurrentLocation(&trailingTypeLoc) || parser.parseType(type) ||
       parser.resolveOperand(addr, type, result.operands))
     return failure();
 
   Type elemTy = getLoadStoreElementType(parser, type, trailingTypeLoc);
 
-  result.attributes = attrs;
   result.addTypes(elemTy);
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Printing/parsing for LLVM::StoreOp.
 //===----------------------------------------------------------------------===//
 
 static void printStoreOp(OpAsmPrinter &p, StoreOp &op) {
   p << op.getOperationName() << ' ' << op.value() << ", " << op.addr();
   p.printOptionalAttrDict(op.getAttrs());
   p << " : " << op.addr().getType();
 }
 
 // <operation> ::= `llvm.store` ssa-use `,` ssa-use attribute-dict? `:` type
 static ParseResult parseStoreOp(OpAsmParser &parser, OperationState &result) {
-  SmallVector<NamedAttribute, 4> attrs;
   OpAsmParser::OperandType addr, value;
   Type type;
   llvm::SMLoc trailingTypeLoc;
 
   if (parser.parseOperand(value) || parser.parseComma() ||
-      parser.parseOperand(addr) || parser.parseOptionalAttrDict(attrs) ||
-      parser.parseColon() || parser.getCurrentLocation(&trailingTypeLoc) ||
-      parser.parseType(type))
+      parser.parseOperand(addr) ||
+      parser.parseOptionalAttrDict(result.attributes) || parser.parseColon() ||
+      parser.getCurrentLocation(&trailingTypeLoc) || parser.parseType(type))
     return failure();
 
   Type elemTy = getLoadStoreElementType(parser, type, trailingTypeLoc);
   if (!elemTy)
     return failure();
 
   if (parser.resolveOperand(value, elemTy, result.operands) ||
       parser.resolveOperand(addr, type, result.operands))
     return failure();
 
-  result.attributes = attrs;
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Printing/parsing for LLVM::CallOp.
 //===----------------------------------------------------------------------===//
 
 static void printCallOp(OpAsmPrinter &p, CallOp &op) {
SmallVector<Type, 8> argTypes(
       llvm::drop_begin(op.getOperandTypes(), isDirect ? 0 : 1));
 
   p << " : " << FunctionType::get(argTypes, resultTypes, op.getContext());
 }
 
 // <operation> ::= `llvm.call` (function-id | ssa-use) `(` ssa-use-list `)`
 //                 attribute-dict? `:` function-type
 static ParseResult parseCallOp(OpAsmParser &parser, OperationState &result) {
-  SmallVector<NamedAttribute, 4> attrs;
   SmallVector<OpAsmParser::OperandType, 8> operands;
   Type type;
   SymbolRefAttr funcAttr;
   llvm::SMLoc trailingTypeLoc;
 
   // Parse an operand list that will, in practice, contain 0 or 1 operand.  In
   // case of an indirect call, there will be 1 operand before `(`.  In case of a
   // direct call, there will be no operands and the parser will stop at the
   // function identifier without complaining.
   if (parser.parseOperandList(operands))
     return failure();
   bool isDirect = operands.empty();
 
   // Optionally parse a function identifier.
   if (isDirect)
-    if (parser.parseAttribute(funcAttr, "callee", attrs))
+    if (parser.parseAttribute(funcAttr, "callee", result.attributes))
       return failure();
 
   if (parser.parseOperandList(operands, OpAsmParser::Delimiter::Paren) ||
-      parser.parseOptionalAttrDict(attrs) || parser.parseColon() ||
+      parser.parseOptionalAttrDict(result.attributes) || parser.parseColon() ||
       parser.getCurrentLocation(&trailingTypeLoc) || parser.parseType(type))
     return failure();
 
   auto funcType = type.dyn_cast<FunctionType>();
   if (!funcType)
     return parser.emitError(trailingTypeLoc, "expected function type");
   if (isDirect) {
     // Make sure types match.
if (isDirect) {
     if (parser.resolveOperand(operands[0], wrappedFuncType, result.operands) ||
         parser.resolveOperands(funcArguments, funcType.getInputs(),
                                parser.getNameLoc(), result.operands))
       return failure();
 
     result.addTypes(llvmResultType);
   }
 
-  result.attributes = attrs;
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Printing/parsing for LLVM::ExtractElementOp.
 //===----------------------------------------------------------------------===//
 // Expects vector to be of wrapped LLVM vector type and position to be of
 // wrapped LLVM i32 type.

 }
 
 // Extract the type at `position` in the wrapped LLVM IR aggregate type
 // `containerType`.  Position is an integer array attribute where each value
 // is a zero-based position of the element in the aggregate type.  Return the
 // resulting type wrapped in MLIR, or nullptr on error.
 static LLVM::LLVMType getInsertExtractValueElementType(OpAsmParser &parser,
                                                        Type containerType,
-                                                       Attribute positionAttr,
+                                                       ArrayAttr positionAttr,
                                                        llvm::SMLoc attributeLoc,
                                                        llvm::SMLoc typeLoc) {
   auto wrappedContainerType = containerType.dyn_cast<LLVM::LLVMType>();
   if (!wrappedContainerType)
     return parser.emitError(typeLoc, "expected LLVM IR Dialect type"), nullptr;
 
-  auto positionArrayAttr = positionAttr.dyn_cast<ArrayAttr>();
-  if (!positionArrayAttr)
-    return parser.emitError(attributeLoc, "expected an array attribute"),
-           nullptr;
-
   // Infer the element type from the structure type: iteratively step inside the
   // type by taking the element type, indexed by the position attribute for
   // structures.  Check the position index before accessing, it is supposed to
   // be in bounds.
-  for (Attribute subAttr : positionArrayAttr) {
+  for (Attribute subAttr : positionAttr) {
     auto positionElementAttr = subAttr.dyn_cast<IntegerAttr>();
     if (!positionElementAttr)
       return parser.emitError(attributeLoc,
                               "expected an array of integer literals"),
              nullptr;
     int position = positionElementAttr.getInt();
     auto *llvmContainerType = wrappedContainerType.getUnderlyingType();
     if (llvmContainerType->isArrayTy()) {
static LLVM::LLVMType getInsertExtractValueElementType(OpAsmParser &parser,
   return wrappedContainerType;
 }
 
 // <operation> ::= `llvm.extractvalue` ssa-use
 //                 `[` integer-literal (`,` integer-literal)* `]`
 //                 attribute-dict? `:` type
 static ParseResult parseExtractValueOp(OpAsmParser &parser,
                                        OperationState &result) {
-  SmallVector<NamedAttribute, 4> attrs;
   OpAsmParser::OperandType container;
   Type containerType;
-  Attribute positionAttr;
+  ArrayAttr positionAttr;
   llvm::SMLoc attributeLoc, trailingTypeLoc;
 
   if (parser.parseOperand(container) ||
       parser.getCurrentLocation(&attributeLoc) ||
-      parser.parseAttribute(positionAttr, "position", attrs) ||
-      parser.parseOptionalAttrDict(attrs) || parser.parseColon() ||
+      parser.parseAttribute(positionAttr, "position", result.attributes) ||
+      parser.parseOptionalAttrDict(result.attributes) || parser.parseColon() ||
       parser.getCurrentLocation(&trailingTypeLoc) ||
       parser.parseType(containerType) ||
       parser.resolveOperand(container, containerType, result.operands))
     return failure();
 
   auto elementType = getInsertExtractValueElementType(
       parser, containerType, positionAttr, attributeLoc, trailingTypeLoc);
   if (!elementType)
     return failure();
 
-  result.attributes = attrs;
   result.addTypes(elementType);
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Printing/parsing for LLVM::InsertElementOp.
 //===----------------------------------------------------------------------===//
 

 
 // <operation> ::= `llvm.insertvaluevalue` ssa-use `,` ssa-use
 //                 `[` integer-literal (`,` integer-literal)* `]`
 //                 attribute-dict? `:` type
 static ParseResult parseInsertValueOp(OpAsmParser &parser,
                                       OperationState &result) {
   OpAsmParser::OperandType container, value;
   Type containerType;
-  Attribute positionAttr;
+  ArrayAttr positionAttr;
   llvm::SMLoc attributeLoc, trailingTypeLoc;
 
   if (parser.parseOperand(value) || parser.parseComma() ||
       parser.parseOperand(container) ||
       parser.getCurrentLocation(&attributeLoc) ||
       parser.parseAttribute(positionAttr, "position", result.attributes) ||
       parser.parseOptionalAttrDict(result.attributes) || parser.parseColon() ||
       parser.getCurrentLocation(&trailingTypeLoc) ||

 }
 
 // <operation> ::= `llvm.shufflevector` ssa-use `, ` ssa-use
 //                 `[` integer-literal (`,` integer-literal)* `]`
 //                 attribute-dict? `:` type
 static ParseResult parseShuffleVectorOp(OpAsmParser &parser,
                                         OperationState &result) {
   llvm::SMLoc loc;
-  SmallVector<NamedAttribute, 4> attrs;
   OpAsmParser::OperandType v1, v2;
-  Attribute maskAttr;
+  ArrayAttr maskAttr;
   Type typeV1, typeV2;
   if (parser.getCurrentLocation(&loc) || parser.parseOperand(v1) ||
       parser.parseComma() || parser.parseOperand(v2) ||
-      parser.parseAttribute(maskAttr, "mask", attrs) ||
-      parser.parseOptionalAttrDict(attrs) || parser.parseColonType(typeV1) ||
-      parser.parseComma() || parser.parseType(typeV2) ||
+      parser.parseAttribute(maskAttr, "mask", result.attributes) ||
+      parser.parseOptionalAttrDict(result.attributes) ||
+      parser.parseColonType(typeV1) || parser.parseComma() ||
+      parser.parseType(typeV2) ||
       parser.resolveOperand(v1, typeV1, result.operands) ||
       parser.resolveOperand(v2, typeV2, result.operands))
     return failure();
   auto wrappedContainerType1 = typeV1.dyn_cast<LLVM::LLVMType>();
   if (!wrappedContainerType1 ||
       !wrappedContainerType1.getUnderlyingType()->isVectorTy())
     return parser.emitError(
         loc, "expected LLVM IR dialect vector type for operand #1");
-  auto vType =
-      LLVMType::getVectorTy(wrappedContainerType1.getVectorElementType(),
-                            maskAttr.cast<ArrayAttr>().size());
-  result.attributes = attrs;
+  auto vType = LLVMType::getVectorTy(
+      wrappedContainerType1.getVectorElementType(), maskAttr.size());
   result.addTypes(vType);
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Implementations for LLVM::LLVMFuncOp.
 //===----------------------------------------------------------------------===//
 

mlir/test/Dialect/LLVMIR/invalid.mlir

 // RUN: mlir-opt %s -split-input-file -verify-diagnostics
 
 // expected-error@+1{{llvm.noalias argument attribute of non boolean type}}
 func @invalid_noalias(%arg0: !llvm.i32 {llvm.noalias = 3}) {
   "llvm.return"() : () -> ()
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 
 // Check that parser errors are properly produced and do not crash the compiler.
 
 // -----
 
 func @icmp_non_string(%arg0 : !llvm.i32, %arg1 : !llvm<"i16">) {
-  // expected-error@+1 {{expected 'predicate' attribute of string type}}
+  // expected-error@+1 {{invalid kind of attribute specified}}
   llvm.icmp 42 %arg0, %arg0 : !llvm.i32
   return
 }
 
 // -----
 
 func @icmp_wrong_string(%arg0 : !llvm.i32, %arg1 : !llvm<"i16">) {
   // expected-error@+1 {{'foo' is an incorrect value of the 'predicate' attribute}}
func @insertvalue_non_llvm_type(%a : i32, %b : i32) {
   llvm.insertvalue %a, %b[0] : i32
 }
 
 // -----
 
 func @insertvalue_non_array_position() {
   // Note the double-type, otherwise attribute parsing consumes the trailing
   // type of the op as the (wrong) attribute type.
-  // expected-error@+1 {{expected an array attribute}}
+  // expected-error@+1 {{invalid kind of attribute specified}}
   llvm.insertvalue %a, %b 0 : i32 : !llvm<"{i32}">
 }
 
 // -----
 
 func @insertvalue_non_integer_position() {
   // expected-error@+1 {{expected an array of integer literals}}
   llvm.insertvalue %a, %b[0.0] : !llvm<"{i32}">
func @extractvalue_non_llvm_type(%a : i32, %b : i32) {
   llvm.extractvalue %b[0] : i32
 }
 
 // -----
 
 func @extractvalue_non_array_position() {
   // Note the double-type, otherwise attribute parsing consumes the trailing
   // type of the op as the (wrong) attribute type.
-  // expected-error@+1 {{expected an array attribute}}
+  // expected-error@+1 {{invalid kind of attribute specified}}
   llvm.extractvalue %b 0 : i32 : !llvm<"{i32}">
 }
 
 // -----
 
 func @extractvalue_non_integer_position() {
   // expected-error@+1 {{expected an array of integer literals}}
   llvm.extractvalue %b[0.0] : !llvm<"{i32}">