[mlir] Change the syntax of dense arrays

[Mogball]

Summary

Follow-up to D123774, where the syntax of dense arrays was discussed. It
was included that the syntax should be changed to array<i32: 1, 2>.
This patch changes the syntax but importantly preserves the [1, 2]
syntax when embedding these attributes in assembly formats through ODS.

[jpienaar]

Thanks for this cleanup!

mlir/include/mlir/IR/BuiltinAttributes.td

179–181

Borderline between dropping : when empty, but consistency in printing parsing pushed towards what you did.

[Mogball]

mlir/include/mlir/IR/BuiltinAttributes.td

179–181

Yes, this looks weird. I honestly prefer array<1, 2, 3> : i8 but that requires parsing ahead...

[mehdi_amini]

Thanks!

[lattner]

Awesome, thank you for pushing this forward. What do y'all think about changing the empty syntax to just array<i8>? It looks weird to have the colon there in the empty case.

mlir/test/IR/attribute.mlir

538

What do y'all think about changing the empty syntax to just array<i8>? It looks weird to have the colon there in the empty case.

[mehdi_amini]

A Space instead of the colon LGTM assuming we won’t have any kind of ambiguity (I don’t think so but …)

[rriddle]

Thanks for tackling this!

The proposed change for the empty syntax LGTM.

mlir/include/mlir/IR/BuiltinAttributes.td

let description = [{
     Syntax:
 
     ```
     dense-array-attribute ::= `[` `:` (integer-type | float-type) tensor-literal `]`
     ```
     Examples:
 
     ```mlir
-    [:i8]
-    [:i32 10, 42]
-    [:f64 42., 12.]
+    array<i8:>
+    array<i32: 10, 42>
+    array<f64: 42., 12.>
     ```
 
     when a specific subclass is used as argument of an operation, the declarative
     assembly will omit the type and print directly:
     ```
     [1, 2, 3]
     ```
   }];

mlir/lib/AsmParser/AttributeParser.cpp

if (parseToken(Token::less, "expected '<' in integer set") ||
         parseToken(Token::greater, "expected '>' in integer set"))
       return Attribute();
     return IntegerSetAttr::get(set);
   }
 
   // Parse an array attribute.
   case Token::l_square: {
     consumeToken(Token::l_square);
-    if (consumeIf(Token::colon))
-      return parseDenseArrayAttr();
     SmallVector<Attribute, 4> elements;
     auto parseElt = [&]() -> ParseResult {
       elements.push_back(parseAttribute());
       return elements.back() ? success() : failure();
     };
 
     if (parseCommaSeparatedListUntil(Token::r_square, parseElt))
       return nullptr;
Attribute Parser::parseAttribute(Type type) {
   // Parse a dense elements attribute.
   case Token::kw_dense:
     return parseDenseElementsAttr(type);
 
   // Parse a dense resource elements attribute.
   case Token::kw_dense_resource:
     return parseDenseResourceElementsAttr(type);
 
+  // Parse a dense array attribute.
+  case Token::kw_array:
+    return parseDenseArrayAttr(type);
+
   // Parse a dictionary attribute.
   case Token::l_brace: {
     NamedAttrList elements;
     if (parseAttributeDict(elements))
       return nullptr;
     return elements.getDictionary(getContext());
   }
 

 class CustomAsmParser : public AsmParserImpl<AsmParser> {
 public:
   CustomAsmParser(Parser &parser)
       : AsmParserImpl<AsmParser>(parser.getToken().getLoc(), parser) {}
 };
 } // namespace
 
 /// Parse a dense array attribute.
-Attribute Parser::parseDenseArrayAttr() {
-  auto typeLoc = getToken().getLoc();
-  auto type = parseType();
-  if (!type)
+Attribute Parser::parseDenseArrayAttr(Type type) {
+  consumeToken(Token::kw_array);
+  SMLoc typeLoc = getToken().getLoc();
+  if (parseToken(Token::less, "expected '<' after 'array'"))
+    return {};
+  if (!type &&
+      (!(type = parseType()) ||
+       parseToken(Token::colon, "expected ':' after dense array type")))
     return {};
   CustomAsmParser parser(*this);
   Attribute result;
   // Check for empty list.
-  bool isEmptyList = getToken().is(Token::r_square);
+  bool isEmptyList = getToken().is(Token::greater);
 
   if (auto intType = type.dyn_cast<IntegerType>()) {
     switch (type.getIntOrFloatBitWidth()) {
     case 1:
       if (isEmptyList)
         result = DenseBoolArrayAttr::get(parser.getContext(), {});
       else
         result = DenseBoolArrayAttr::parseWithoutBraces(parser, Type{});
if (auto intType = type.dyn_cast<IntegerType>()) {
     default:
       emitError(typeLoc, "expected f32 or f64 but got: ") << type;
       return {};
     }
   } else {
     emitError(typeLoc, "expected integer or float type, got: ") << type;
     return {};
   }
-  if (!consumeIf(Token::r_square)) {
-    emitError("expected ']' to close an array attribute");
+  if (parseToken(Token::greater, "expected '>' to close an array attribute"))
     return {};
-  }
   return result;
 }
 
 /// Parse a dense elements attribute.
 Attribute Parser::parseDenseElementsAttr(Type attrType) {
   auto attribLoc = getToken().getLoc();
   consumeToken(Token::kw_dense);
   if (parseToken(Token::less, "expected '<' after 'dense'"))

mlir/lib/AsmParser/Parser.h

public:
   /// Parse a dense elements attribute.
   Attribute parseDenseElementsAttr(Type attrType);
   ShapedType parseElementsLiteralType(Type type);
 
   /// Parse a dense resource elements attribute.
   Attribute parseDenseResourceElementsAttr(Type attrType);
 
   /// Parse a DenseArrayAttr.
-  Attribute parseDenseArrayAttr();
+  Attribute parseDenseArrayAttr(Type type);
 
   /// Parse a sparse elements attribute.
   Attribute parseSparseElementsAttr(Type attrType);
 
   //===--------------------------------------------------------------------===//
   // Location Parsing
   //===--------------------------------------------------------------------===//
 

mlir/lib/AsmParser/TokenKinds.def

 TOK_PUNCTUATION(file_metadata_end, "#-}")
 
 // Keywords.  These turn "foo" into Token::kw_foo enums.
 
 // NOTE: Please key these alphabetized to make it easier to find something in
 // this list and to cater to OCD.
 TOK_KEYWORD(affine_map)
 TOK_KEYWORD(affine_set)
+TOK_KEYWORD(array)
 TOK_KEYWORD(attributes)
 TOK_KEYWORD(bf16)
 TOK_KEYWORD(ceildiv)
 TOK_KEYWORD(complex)
 TOK_KEYWORD(dense)
 TOK_KEYWORD(dense_resource)
 TOK_KEYWORD(f16)
 TOK_KEYWORD(f32)

mlir/lib/IR/AsmPrinter.cpp

if (printerFlags.shouldElideElementsAttr(sparseEltAttr.getIndices()) ||
         printDenseIntOrFPElementsAttr(indices, /*allowHex=*/false);
         os << ", ";
         printDenseElementsAttr(sparseEltAttr.getValues(), /*allowHex=*/true);
       }
       os << '>';
     }
   } else if (auto denseArrayAttr = attr.dyn_cast<DenseArrayBaseAttr>()) {
     typeElision = AttrTypeElision::Must;
-    os << "[:" << denseArrayAttr.getType().getElementType();
+    os << "array<" << denseArrayAttr.getType().getElementType() << ":";
     if (denseArrayAttr.size())
       os << " ";
     denseArrayAttr.printWithoutBraces(os);
-    os << "]";
+    os << ">";
   } else if (auto resourceAttr = attr.dyn_cast<DenseResourceElementsAttr>()) {
     os << "dense_resource<";
     printResourceHandle(resourceAttr.getRawHandle());
     os << ">";
   } else if (auto locAttr = attr.dyn_cast<LocationAttr>()) {
     printLocation(locAttr);
   } else {
     llvm::report_fatal_error("Unknown builtin attribute");

mlir/test/Dialect/LLVMIR/dynamic-gep-index.mlir

 // RUN: mlir-opt %s | FileCheck %s
 
 module attributes {dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<i1, dense<8> : vector<2xi32>>, #dlti.dl_entry<i8, dense<8> : vector<2xi32>>, #dlti.dl_entry<i16, dense<16> : vector<2xi32>>, #dlti.dl_entry<i32, dense<32> : vector<2xi32>>, #dlti.dl_entry<i64, dense<[32, 64]> : vector<2xi32>>, #dlti.dl_entry<f16, dense<16> : vector<2xi32>>, #dlti.dl_entry<f64, dense<64> : vector<2xi32>>, #dlti.dl_entry<f128, dense<128> : vector<2xi32>>>} {
   // CHECK: llvm.func @foo(%[[ARG0:.+]]: !llvm.ptr<struct<"my_struct", {{.+}}>>, %[[ARG1:.+]]: i32)
   llvm.func @foo(%arg0: !llvm.ptr<struct<"my_struct", (struct<"sub_struct", (i32, i8)>, array<4 x i32>)>>, %arg1: i32) {
     // CHECK: %[[C0:.+]] = llvm.mlir.constant(0 : i32)
     %0 = llvm.mlir.constant(0 : i32) : i32
     // CHECK: llvm.getelementptr %[[ARG0]][%[[C0]], 1, %[[ARG1]]]
-    %1 = "llvm.getelementptr"(%arg0, %0, %arg1) {rawConstantIndices = [:i32 -2147483648, 1, -2147483648]} : (!llvm.ptr<struct<"my_struct", (struct<"sub_struct", (i32, i8)>, array<4 x i32>)>>, i32, i32) -> !llvm.ptr<i32>
+    %1 = "llvm.getelementptr"(%arg0, %0, %arg1) {rawConstantIndices = array<i32: -2147483648, 1, -2147483648>} : (!llvm.ptr<struct<"my_struct", (struct<"sub_struct", (i32, i8)>, array<4 x i32>)>>, i32, i32) -> !llvm.ptr<i32>
     llvm.return
   }
 }

mlir/test/Dialect/LLVMIR/invalid.mlir

func.func @gep_non_function_type(%pos : i64, %base : !llvm.ptr<f32>) {
   // expected-error@+1 {{invalid kind of type specified}}
   llvm.getelementptr %base[%pos] : !llvm.ptr<f32>
 }
 
 // -----
 
 func.func @gep_too_few_dynamic(%base : !llvm.ptr<f32>) {
   // expected-error@+1 {{expected as many dynamic indices as specified in 'rawConstantIndices'}}
-  %1 = "llvm.getelementptr"(%base) {rawConstantIndices = [:i32 -2147483648]} : (!llvm.ptr<f32>) -> !llvm.ptr<f32>
+  %1 = "llvm.getelementptr"(%base) {rawConstantIndices = array<i32: -2147483648>} : (!llvm.ptr<f32>) -> !llvm.ptr<f32>
 }
 
 // -----
 
 func.func @load_non_llvm_type(%foo : memref<f32>) {
   // expected-error@+1 {{expected LLVM pointer type}}
   llvm.load %foo : memref<f32>
 }
func.func @insertvalue_wrong_nesting() {
   // expected-error@+2 {{expected LLVM IR structure/array type}}
   llvm.insertvalue %a, %b[0,0] : !llvm.struct<(i32)>
 }
 
 // -----
 
 func.func @insertvalue_invalid_type(%a : !llvm.array<1 x i32>) -> !llvm.array<1 x i32> {
   // expected-error@+1 {{'llvm.insertvalue' op Type mismatch: cannot insert '!llvm.array<1 x i32>' into '!llvm.array<1 x i32>'}}
-  %b = "llvm.insertvalue"(%a, %a) {position = [:i64 0]} : (!llvm.array<1 x i32>, !llvm.array<1 x i32>) -> !llvm.array<1 x i32>
+  %b = "llvm.insertvalue"(%a, %a) {position = array<i64: 0>} : (!llvm.array<1 x i32>, !llvm.array<1 x i32>) -> !llvm.array<1 x i32>
   return %b : !llvm.array<1 x i32>
 }
 
 // -----
 
 func.func @extractvalue_invalid_type(%a : !llvm.array<4 x vector<8xf32>>) -> !llvm.array<4 x vector<8xf32>> {
   // expected-error@+1 {{'llvm.extractvalue' op Type mismatch: extracting from '!llvm.array<4 x vector<8xf32>>' should produce 'vector<8xf32>' but this op returns '!llvm.array<4 x vector<8xf32>>'}}
-  %b = "llvm.extractvalue"(%a) {position = [:i64 1]}
+  %b = "llvm.extractvalue"(%a) {position = array<i64: 1>}
             : (!llvm.array<4 x vector<8xf32>>) -> !llvm.array<4 x vector<8xf32>>
   return %b : !llvm.array<4 x vector<8xf32>>
 }
 
 
 // -----
 
 func.func @extractvalue_non_llvm_type(%a : i32, %b : tensor<*xi32>) {

mlir/test/IR/attribute.mlir

 // -----
 
 //===----------------------------------------------------------------------===//
 // Test DenseArrayAttr
 //===----------------------------------------------------------------------===//
 
 // CHECK-LABEL: func @dense_array_attr
 func.func @dense_array_attr() attributes {
-// CHECK-SAME: emptyf32attr = [:f32],
-               emptyf32attr = [:f32],
-// CHECK-SAME: emptyf64attr = [:f64],
-               emptyf64attr = [:f64],
-// CHECK-SAME: emptyi16attr = [:i16],
-               emptyi16attr = [:i16],
-// CHECK-SAME: emptyi1attr = [:i1],
-               emptyi1attr = [:i1],
-// CHECK-SAME: emptyi32attr = [:i32],
-               emptyi32attr = [:i32],
-// CHECK-SAME: emptyi64attr = [:i64],
-               emptyi64attr = [:i64],
-// CHECK-SAME: emptyi8attr = [:i8],
-               emptyi8attr = [:i8],
-// CHECK-SAME: f32attr = [:f32 1.024000e+03, 4.530000e+02, -6.435000e+03],
-               f32attr = [:f32 1024., 453., -6435.],
-// CHECK-SAME: f64attr = [:f64 -1.420000e+02],
-               f64attr = [:f64 -142.],
-// CHECK-SAME: i16attr = [:i16 3, 5, -4, 10],
-               i16attr = [:i16 3, 5, -4, 10],
-// CHECK-SAME: i1attr = [:i1 true, false, true],
-               i1attr = [:i1 true, false, true],
-// CHECK-SAME: i32attr = [:i32 1024, 453, -6435],
-               i32attr = [:i32 1024, 453, -6435],
-// CHECK-SAME: i64attr = [:i64 -142],
-               i64attr = [:i64 -142],
-// CHECK-SAME: i8attr = [:i8 1, -2, 3]
-               i8attr = [:i8 1, -2, 3]
+// CHECK-SAME: emptyf32attr = array<f32:>,
+               emptyf32attr = array<f32:>,
+// CHECK-SAME: emptyf64attr = array<f64:>,
+               emptyf64attr = array<f64:>,
+// CHECK-SAME: emptyi16attr = array<i16:>,
+               emptyi16attr = array<i16:>,
+// CHECK-SAME: emptyi1attr = array<i1:>,
+               emptyi1attr = array<i1:>,
+// CHECK-SAME: emptyi32attr = array<i32:>,
+               emptyi32attr = array<i32:>,
+// CHECK-SAME: emptyi64attr = array<i64:>,
+               emptyi64attr = array<i64:>,
+// CHECK-SAME: emptyi8attr = array<i8:>,
+               emptyi8attr = array<i8:>,
+// CHECK-SAME: f32attr = array<f32: 1.024000e+03, 4.530000e+02, -6.435000e+03>,
+               f32attr = array<f32: 1024., 453., -6435.>,
+// CHECK-SAME: f64attr = array<f64: -1.420000e+02>,
+               f64attr = array<f64: -142.>,
+// CHECK-SAME: i16attr = array<i16: 3, 5, -4, 10>,
+               i16attr = array<i16: 3, 5, -4, 10>,
+// CHECK-SAME: i1attr = array<i1: true, false, true>,
+               i1attr = array<i1: true, false, true>,
+// CHECK-SAME: i32attr = array<i32: 1024, 453, -6435>,
+               i32attr = array<i32: 1024, 453, -6435>,
+// CHECK-SAME: i64attr = array<i64: -142>,
+               i64attr = array<i64: -142>,
+// CHECK-SAME: i8attr = array<i8: 1, -2, 3>
+               i8attr = array<i8: 1, -2, 3>
  } {
 // CHECK:  test.dense_array_attr
   test.dense_array_attr
 // CHECK-SAME: i1attr = [true, false, true]
                i1attr = [true, false, true]
 // CHECK-SAME: i8attr = [1, -2, 3]
                i8attr = [1, -2, 3]
 // CHECK-SAME: i16attr = [3, 5, -4, 10]

mlir/test/IR/elements-attr-interface.mlir

 // Check that we don't crash on empty element attributes.
 // expected-error@below {{Test iterating `int64_t`: }}
 // expected-error@below {{Test iterating `uint64_t`: }}
 // expected-error@below {{Test iterating `APInt`: }}
 // expected-error@below {{Test iterating `IntegerAttr`: }}
 arith.constant dense<> : tensor<0xi64>
 
 // expected-error@below {{Test iterating `bool`: true, false, true, false, true, false}}
-arith.constant [:i1 true, false, true, false, true, false]
+arith.constant array<i1: true, false, true, false, true, false>
 // expected-error@below {{Test iterating `int8_t`: 10, 11, -12, 13, 14}}
-arith.constant [:i8 10, 11, -12, 13, 14]
+arith.constant array<i8: 10, 11, -12, 13, 14>
 // expected-error@below {{Test iterating `int16_t`: 10, 11, -12, 13, 14}}
-arith.constant [:i16 10, 11, -12, 13, 14]
+arith.constant array<i16: 10, 11, -12, 13, 14>
 // expected-error@below {{Test iterating `int32_t`: 10, 11, -12, 13, 14}}
-arith.constant [:i32 10, 11, -12, 13, 14]
+arith.constant array<i32: 10, 11, -12, 13, 14>
 // expected-error@below {{Test iterating `int64_t`: 10, 11, -12, 13, 14}}
-arith.constant [:i64 10, 11, -12, 13, 14]
+arith.constant array<i64: 10, 11, -12, 13, 14>
 // expected-error@below {{Test iterating `float`: 10.00, 11.00, -12.00, 13.00, 14.00}}
-arith.constant [:f32 10., 11., -12., 13., 14.]
+arith.constant array<f32: 10., 11., -12., 13., 14.>
 // expected-error@below {{Test iterating `double`: 10.00, 11.00, -12.00, 13.00, 14.00}}
-arith.constant [:f64 10., 11., -12., 13., 14.]
+arith.constant array<f64: 10., 11., -12., 13., 14.>
 
 // Check that we handle an external constant parsed from the config.
 // expected-error@below {{Test iterating `int64_t`: unable to iterate type}}
 // expected-error@below {{Test iterating `uint64_t`: 1, 2, 3}}
 // expected-error@below {{Test iterating `APInt`: unable to iterate type}}
 // expected-error@below {{Test iterating `IntegerAttr`: unable to iterate type}}
 arith.constant #test.e1di64_elements<blob1> : tensor<3xi64>
 
 {-#
   dialect_resources: {
     test: {
       blob1: "0x08000000010000000000000002000000000000000300000000000000"
     }
   }
 #-}