MLIR-ODS DenseXXXArrayAttr + ArrayMinCount seem like a footgun

MLIR
1

I was looking at @Mogball 's modernization PR here ⚙ D131971 [mlir][LLVMIR] "Modernize" LLVM insert/extract element operations and wanted to start using the nicer DenseI64ArrayAttr in my code too.

However I realized we may be reaching the limits of the Tablegen parts of the infra quite quickly.

ConfinedAttr<OptionalAttr<DenseI64ArrayAttr>, [ArrayMinCount<3>]>

is a pretty bad footgun that generates:

if (attr && !(((attr.isa<::mlir::DenseI64ArrayAttr>())) && ((attr.cast<::mlir::ArrayAttr>().size() >= 3)))) {
    return op->emitOpError("attribute '") << attrName
        << "' failed to satisfy constraint: i64 dense array attribute with at least 3 elements";
  }

This crashes at runtime on the attr.cast.

What is the recommended way forward ?

2

You are trying to confine an array with an integer constraint. That doesn’t make sense indeed . The error could be improved but at that level the C++ types involved and rules regarding them is not trivial to deduce (I see this less as a footgun and more as inconsistent). Wouldn’t you need an all of constraint that would verify for all in the container that the elementwise constraint holds?

3

Sorry I ended up pasting a wrong test case, edited.

The issue here is that DenseI64ArrayAttr is not a ArrayAttr and behind the Tablegen → C++ indirection this is not immediately clear and seems to make the API hard to use properly.

I was thinking of trying to implement ArrayIntNonNegative and friends for my use cases but it seems dangerous atm, so I thought I’d ask here first.

4

I actually don’t understand the && instead of || in the test? I guess I need to read the TableGen code to figure out how we get there…

5

I am interpreting this as the conjunction of 3 constraints:

  1. Optional, and
  2. DenseI64ArrayAttr, and
  3. ArrayMinCount<3> which casts to ArrayAttr, this one is added by ConfinedAttr.

I think I would be surprised to see a ||.

6

Ah right I misread the number of parentheses, and didn’t see the different class name between isa and cast.
You’re right that this is a problem with the core infra and how we name things here.

7

I have been always bothered by the name ArrayAttr because I am used to associating “array” with a container of identically-typed objects, yet ArrayAttr can have elements of different types. IMO, it is more of a TupleAttr given that it is also immutable, whereas the Dense*ArrayAttr is actually an ArrayAttr. I can’t even start imagining the amount of churn caused by a rename :sweat_smile:.

A stupid solution is to update ArrayMinCount to emit code that dispatches based on type (attr.isa<ArrayAttr>() ? attr.cast<ArrayAttr>().size() : attr.cast<DenseArrayBaseAttr>().size()), which is not exactly elegant or efficient.

Preventing such footguns may require some work at the tablegen level where we could, for example, extend AttributeConstraint to keep track of the supported base attribute classes and check that is it only applied to those or their subclasses at tablegen level.

8

I would think that we should instead use the ODS type (but how to access the attribute or its type post-cast in the constraint?) and dispatch that way.

9

:thinking:

ArrayAttr has historically been abused to behave like a true “array”, in which all elements are the same kind. I wouldn’t be surprised if every attribute constrained by ArrayMinCount today would make more sense as a dense array or a custom attribute.