[RFC] Introduce a new Dense Array attribute

MLIR
1

The existing DenseElementAttr is convenient but exposes a very complex C++ API. A lot of it stems from the fact that it supports splat, which was motivated by the desire to handle large constant.
Also it is hard to subclass: an op that just want an array of i64 (for example any attribute that refers to the dimensions, like in a reshape or transpose operation) will still expose and API based on the generic DenseElementAttr: this leads to a lot of inefficiency where people iterate through it using APInt/APFloat.

This new attribute is similar to DenseElementsAttr but does not support
splat. As such it has a much simpler API and does not need any smart
iterator: it exposes direct ArrayRef access.

A new syntax is introduced so that the generic printing/parsing looks
like:

   [:i16 1, -2, 3]

It opens on a square bracket like the ArrayAttr, but is followed by a colon to distinguish the two. The colon is followed by the element type
(supported are I8, I16, I32, I64, F32, F64) and then the comma-separated list for the data as usual.

This is particularly convenient for attributes intended to be small,
like those referring to shapes.
For example a transpose operation with a dims attribute could be
defined as such:

let arguments = (ins AnyTensor:$input, DenseI64ArrayAttr:$dims); let assemblyFormat = "$input `dims` `=` $dims attr-dict : type($input)";

And printed this way (we elide the element type here):

transpose %input dims = [0, 2, 1] : tensor<2x3x4xf32>

The attribute is directly parsed/printed as data in between square brackets without any other type indication.

Also and more importantly, the C++ API for TransposeOps::getDims() would just directly return an ArrayRef<int64>.

Revision: āš™ D123774 Introduce a new Dense Array attribute (~400 lines)

I had updated TOSA with an earlier revision of this patch in December, in case you want to see the actual effect on the C++ API that motivated this work: āš™ D116394 Adopt DimensionListAttr in TOSA op definition (NFC)

5 Likes
2

I am very very very +1 on this. Iā€™d argue we should sugar the generic syntax a bit more, maybe to ā€˜array<i16, 1, 2, 3, 4>ā€˜.

The other nice thing that you didnā€™t mention is that it cleanly separates the shape out of a ā€œtensor.constantā€ like operation, putting that shape in the type of the op result, instead of duplicating it into the initializer element storage. The current conflation only makes sense for dense storage and there are lots of other layouts and other considerations that are better put in the op.

I would love to see this replace DenseElementAttr in general over time, the old thing has way too much magic and behavior that is special cases into it. This is far more general and useful. I love it,

-Chris

3

Iā€™d also opine that DenseElementAttr isnā€™t the best way handle splats anyway. Splats are such an important special case that they should get special treatment, not just being implicitly expanded by the compiler logic on accident.

With your proposed attribute, instead of having a tensor.constant that has both dense and splat forms, you can have a tensor.constant and a separate tensor.constant.splat, the later of which takes a scalar attribute.

If you prefer one op for both cases, you can also have your tensor.constant just take your array attribute and use the ā€œwhen the array holds one element, it is a splatā€ approach, instead of requiring that the length of the array = num elements in the resultant tensor.

4

Yes, the tradeoff is that it would push some of the complexity on the client: you canā€™t just get an ArrayRef and handle it: if it has a single element you also need some more contextual information (like the type of the tensor.constant to return) to know it is a splat.
But that seems probably better compared to the complexity we have around DenseElementAttr right now!

5

Yeah, to be clear, I personally donā€™t prefer the ā€œone op for both casesā€ approach. It is better to make different things be different in my experience that trying to make them falsely the same.

2 Likes
6

One other related thing Iā€™d suggest: drop support for any non c types: treat the ā€œtypeā€ as a storage type instead of trying to pretend we are doing framework specific memory encoding.

Specifically I would not support i1/i2/ā€¦/i7 sorts of things or i24. Treat ArrayRef bool as an api convenience that stores the data as 0/1 i8s

2 Likes
7

I updated the proposed syntax (edited the original post accordingly)

8

Patch has landed in: http://github.com/llvm/llvm-project/commit/508eb41d82ca

2 Likes
9

Awesome, very excited by this, thank you!