[mlir] Allow empty lists for DenseArrayAttr.

[rriddle]

mlir/lib/IR/BuiltinAttributes.cpp

919

Why is it allocating one when it's empty? I would expect this to call Base::get with an empty ArrayRef.

[akuegel]

mlir/lib/IR/BuiltinAttributes.cpp

919

This is to avoid an assertion error in VectorType::get.
But a better fix is to not fire the assertion (if it turns out to be unnecessary).
But a similar workaround was already used in the patch where VectorType support for empty shapes was added:
https://reviews.llvm.org/D114387
(TypeCoverter.cpp)
I have already asked Nicolas whether VectorType::get can be fixed. If yes, then these two lines will not be needed.

[rriddle]

mlir/lib/IR/BuiltinAttributes.cpp

919

It's not really a valid workaround though, because it's not creating what the user intended (meaning any number of expected invariants are broken, e.g. most of the ElementsAttr are not going to work as expected). Given that VectorType can't support zero elements, we should switch this to use TensorType, just not support empty lists (which is kind of weird), or let VectorType have zero elements.

[akuegel]

mlir/lib/IR/BuiltinAttributes.cpp

919

As far as I understand Nicolas, the intent is to let VectorType have zero elements.

@nicolasvasilache did I understand that right? And if yes, would you be willing to take a look what would be needed to avoid the assert?

[nicolasvasilache]

mlir/lib/IR/BuiltinAttributes.cpp

919

But a similar workaround was already used in the patch where VectorType support for empty shapes was added:
https://reviews.llvm.org/D114387

You're confusing MLIR vector and LLVM vectors: MLIR vector support 0-D and should work with empty in general, as I mentioned separately they already do.
E.g. in that same PR you link we have: if (vectorType.getRank() == 0) { 0-D vectors are a thing in MLIR.

However LLVM does not know about this and we must convert to llvm.vector<1xT>since we use the same MLIR VectorType representation for MLIR and LLVM the type conversion when lowering to LLVM is vector<T> (for MLIR) -> vector<1xT> (for LLVM).

I am still unclear what problem you are hitting, can you please send a standalone .mlir to repro?

[akuegel]

Fix creating of VectorType when we have 0 elements.

[nicolasvasilache]

mlir/lib/IR/BuiltinAttributes.cpp

919

ok so from offline discussion it seems you are trying to build vector<0xT>; this is indeed illegal, you want to build vector<T> for such cases with an empty shape.

[akuegel]

Thanks Nicolas for pointing out the real problem: we should not use VectorType::get(0, elementType) for 0-d vector. I have rewritten this part in BuiltinAttributes.cpp to already pass an ArrayRef to the getShapedType methods.

[nicolasvasilache]

ok, works for me now, please wait for River to stamp

[rriddle]

mlir/test/IR/attribute.mlir

525

Can we drop the empty space from the syntax?

[akuegel]

Get rid of unnecessary space when printing an empty list with type.

[akuegel]

Thanks for the review, and for pointing out a better way than the workaround I had :)

mlir/lib/IR/AsmPrinter.cpp

if (printerFlags.shouldElideElementsAttr(sparseEltAttr.getIndices()) ||
         printDenseElementsAttr(sparseEltAttr.getValues(), /*allowHex=*/true);
       }
       os << '>';
     }
   } else if (auto denseArrayAttr = attr.dyn_cast<DenseArrayBaseAttr>()) {
     typeElision = AttrTypeElision::Must;
     switch (denseArrayAttr.getElementType()) {
     case DenseArrayBaseAttr::EltType::I8:
-      os << "[:i8 ";
+      os << "[:i8";
       break;
     case DenseArrayBaseAttr::EltType::I16:
-      os << "[:i16 ";
+      os << "[:i16";
       break;
     case DenseArrayBaseAttr::EltType::I32:
-      os << "[:i32 ";
+      os << "[:i32";
       break;
     case DenseArrayBaseAttr::EltType::I64:
-      os << "[:i64 ";
+      os << "[:i64";
       break;
     case DenseArrayBaseAttr::EltType::F32:
-      os << "[:f32 ";
+      os << "[:f32";
       break;
     case DenseArrayBaseAttr::EltType::F64:
-      os << "[:f64 ";
+      os << "[:f64";
       break;
     }
+    if (denseArrayAttr.getType().cast<ShapedType>().getRank())
+      os << " ";
     denseArrayAttr.printWithoutBraces(os);
     os << "]";
   } else if (auto locAttr = attr.dyn_cast<LocationAttr>()) {
     printLocation(locAttr);
   } else {
     llvm::report_fatal_error("Unknown builtin attribute");
   }
   // Don't print the type if we must elide it, or if it is a None type.

mlir/lib/IR/BuiltinAttributes.cpp

Attribute DenseArrayAttr<T>::parseWithoutBraces(AsmParser &parser,
   return get(parser.getContext(), data);
 }
 
 /// Parse a DenseArrayAttr: `[ 1, 2, 3 ]`
 template <typename T>
 Attribute DenseArrayAttr<T>::parse(AsmParser &parser, Type odsType) {
   if (parser.parseLSquare())
     return {};
+  // Handle empty list case.
+  if (succeeded(parser.parseOptionalRSquare()))
+    return get(parser.getContext(), {});
   Attribute result = parseWithoutBraces(parser, odsType);
   if (parser.parseRSquare())
     return {};
   return result;
 }
 
 /// Conversion from DenseArrayAttr<T> to ArrayRef<T>.
 template <typename T>
 DenseArrayAttr<T>::operator ArrayRef<T>() const {
   ArrayRef<char> raw = getImpl()->elements;
   assert((raw.size() % sizeof(T)) == 0);
   return ArrayRef<T>(reinterpret_cast<const T *>(raw.data()),
                      raw.size() / sizeof(T));
 }
 
 namespace {
 /// Mapping from C++ element type to MLIR DenseArrayAttr internals.
 template <typename T>
 struct denseArrayAttrEltTypeBuilder;
 template <>
 struct denseArrayAttrEltTypeBuilder<int8_t> {
   constexpr static auto eltType = DenseArrayBaseAttr::EltType::I8;
-  static ShapedType getShapedType(MLIRContext *context, int64_t shape) {
+  static ShapedType getShapedType(MLIRContext *context,
+                                  ArrayRef<int64_t> shape) {
     return VectorType::get(shape, IntegerType::get(context, 8));
   }
 };
 template <>
 struct denseArrayAttrEltTypeBuilder<int16_t> {
   constexpr static auto eltType = DenseArrayBaseAttr::EltType::I16;
-  static ShapedType getShapedType(MLIRContext *context, int64_t shape) {
+  static ShapedType getShapedType(MLIRContext *context,
+                                  ArrayRef<int64_t> shape) {
     return VectorType::get(shape, IntegerType::get(context, 16));
   }
 };
 template <>
 struct denseArrayAttrEltTypeBuilder<int32_t> {
   constexpr static auto eltType = DenseArrayBaseAttr::EltType::I32;
-  static ShapedType getShapedType(MLIRContext *context, int64_t shape) {
+  static ShapedType getShapedType(MLIRContext *context,
+                                  ArrayRef<int64_t> shape) {
     return VectorType::get(shape, IntegerType::get(context, 32));
   }
 };
 template <>
 struct denseArrayAttrEltTypeBuilder<int64_t> {
   constexpr static auto eltType = DenseArrayBaseAttr::EltType::I64;
-  static ShapedType getShapedType(MLIRContext *context, int64_t shape) {
+  static ShapedType getShapedType(MLIRContext *context,
+                                  ArrayRef<int64_t> shape) {
     return VectorType::get(shape, IntegerType::get(context, 64));
   }
 };
 template <>
 struct denseArrayAttrEltTypeBuilder<float> {
   constexpr static auto eltType = DenseArrayBaseAttr::EltType::F32;
-  static ShapedType getShapedType(MLIRContext *context, int64_t shape) {
+  static ShapedType getShapedType(MLIRContext *context,
+                                  ArrayRef<int64_t> shape) {
     return VectorType::get(shape, Float32Type::get(context));
   }
 };
 template <>
 struct denseArrayAttrEltTypeBuilder<double> {
   constexpr static auto eltType = DenseArrayBaseAttr::EltType::F64;
-  static ShapedType getShapedType(MLIRContext *context, int64_t shape) {
+  static ShapedType getShapedType(MLIRContext *context,
+                                  ArrayRef<int64_t> shape) {
     return VectorType::get(shape, Float64Type::get(context));
   }
 };
 } // namespace
 
 /// Builds a DenseArrayAttr<T> from an ArrayRef<T>.
 template <typename T>
 DenseArrayAttr<T> DenseArrayAttr<T>::get(MLIRContext *context,
                                          ArrayRef<T> content) {
-  auto shapedType =
-      denseArrayAttrEltTypeBuilder<T>::getShapedType(context, content.size());
+  auto size = static_cast<int64_t>(content.size());
+  auto shapedType = denseArrayAttrEltTypeBuilder<T>::getShapedType(
+      context, size ? ArrayRef<int64_t>{size} : ArrayRef<int64_t>{});
   auto eltType = denseArrayAttrEltTypeBuilder<T>::eltType;
   auto rawArray = ArrayRef<char>(reinterpret_cast<const char *>(content.data()),
                                  content.size() * sizeof(T));
   return Base::get(context, shapedType, eltType, rawArray)
       .template cast<DenseArrayAttr<T>>();
 }
 
 template <typename T>

mlir/lib/Parser/AttributeParser.cpp

 /// Parse a dense array attribute.
 Attribute Parser::parseDenseArrayAttr() {
   auto typeLoc = getToken().getLoc();
   auto type = parseType();
   if (!type)
     return {};
   CustomAsmParser parser(*this);
   Attribute result;
+  // Check for empty list.
+  bool isEmptyList = getToken().is(Token::r_square);
+
   if (auto intType = type.dyn_cast<IntegerType>()) {
     switch (type.getIntOrFloatBitWidth()) {
     case 8:
+      if (isEmptyList)
+        result = DenseI8ArrayAttr::get(parser.getContext(), {});
+      else
         result = DenseI8ArrayAttr::parseWithoutBraces(parser, Type{});
       break;
     case 16:
+      if (isEmptyList)
+        result = DenseI16ArrayAttr::get(parser.getContext(), {});
+      else
         result = DenseI16ArrayAttr::parseWithoutBraces(parser, Type{});
       break;
     case 32:
+      if (isEmptyList)
+        result = DenseI32ArrayAttr::get(parser.getContext(), {});
+      else
         result = DenseI32ArrayAttr::parseWithoutBraces(parser, Type{});
       break;
     case 64:
+      if (isEmptyList)
+        result = DenseI64ArrayAttr::get(parser.getContext(), {});
+      else
         result = DenseI64ArrayAttr::parseWithoutBraces(parser, Type{});
       break;
     default:
       emitError(typeLoc, "expected i8, i16, i32, or i64 but got: ") << type;
       return {};
     }
   } else if (auto floatType = type.dyn_cast<FloatType>()) {
     switch (type.getIntOrFloatBitWidth()) {
     case 32:
+      if (isEmptyList)
+        result = DenseF32ArrayAttr::get(parser.getContext(), {});
+      else
         result = DenseF32ArrayAttr::parseWithoutBraces(parser, Type{});
       break;
     case 64:
+      if (isEmptyList)
+        result = DenseF64ArrayAttr::get(parser.getContext(), {});
+      else
         result = DenseF64ArrayAttr::parseWithoutBraces(parser, Type{});
       break;
     default:
       emitError(typeLoc, "expected f32 or f64 but got: ") << type;
       return {};
     }
   } else {
     emitError(typeLoc, "expected integer or float type, got: ") << type;
     return {};

mlir/test/IR/attribute.mlir

 
 // -----
 
 //===----------------------------------------------------------------------===//
 // Test DenseArrayAttr
 //===----------------------------------------------------------------------===//
 
 // CHECK-LABEL: func @dense_array_attr
-func.func @dense_array_attr() attributes{ 
+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: 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: i32attr = [:i32 1024, 453, -6435],
                i32attr = [:i32 1024, 453, -6435],

 // CHECK-SAME: i32attr = [1024, 453, -6435]
                i32attr = [1024, 453, -6435]
 // CHECK-SAME: i64attr = [-142]
                i64attr = [-142]
 // CHECK-SAME: f32attr = [1.024000e+03, 4.530000e+02, -6.435000e+03]
                f32attr = [1024., 453., -6435.]
 // CHECK-SAME: f64attr = [-1.420000e+02]
                f64attr = [-142.]
+// CHECK-SAME: emptyattr = []
+               emptyattr = []
   return
 }
 
 // -----
 
 //===----------------------------------------------------------------------===//
 // Test SymbolRefAttr
 //===----------------------------------------------------------------------===//

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

 
 def DenseArrayAttrOp : TEST_Op<"dense_array_attr"> {
   let arguments = (ins
     DenseI8ArrayAttr:$i8attr,
     DenseI16ArrayAttr:$i16attr,
     DenseI32ArrayAttr:$i32attr,
     DenseI64ArrayAttr:$i64attr,
     DenseF32ArrayAttr:$f32attr,
-    DenseF64ArrayAttr:$f64attr
+    DenseF64ArrayAttr:$f64attr,
+    DenseI32ArrayAttr:$emptyattr
   );
   let assemblyFormat = [{
    `i8attr` `=` $i8attr `i16attr` `=` $i16attr `i32attr` `=` $i32attr
    `i64attr` `=` $i64attr  `f32attr` `=` $f32attr `f64attr` `=` $f64attr
+   `emptyattr` `=` $emptyattr
    attr-dict
   }];
 }
 
 //===----------------------------------------------------------------------===//
 // Test Enum Attributes
 //===----------------------------------------------------------------------===//