Page MenuHomePhabricator

Add a builtin DimensionListAttribute intended to handle the many ops that just want a small array of int64_t as an attribute
Needs ReviewPublic

Authored by mehdi_amini on Dec 29 2021, 9:59 PM.

Details

Summary

Right now the default for such case is DenseIntElementAttr, however because it
is so generic its API is quite hard to use and it is easy to either materialize
individual elements as individual IntegerAttr or as APInt. It is also not
possible to easily get an ArrayRef<int64>.

D116394 is an illustration of the effect of this new attribute on a recent part of
the codebase (the TOSA dialect). There is saves multiple traversals of ArrayAttr
and local creation of SmallVector<int64_t> in various situation.

Diff Detail

Event Timeline

mehdi_amini created this revision.Dec 29 2021, 9:59 PM
mehdi_amini requested review of this revision.Dec 29 2021, 9:59 PM
mehdi_amini edited the summary of this revision. (Show Details)Dec 29 2021, 10:35 PM
mehdi_amini edited the summary of this revision. (Show Details)

Fix CMakeLists.txt

mehdi_amini edited the summary of this revision. (Show Details)Dec 29 2021, 10:44 PM
Mogball requested changes to this revision.Dec 30 2021, 9:05 AM
Mogball added inline comments.
mlir/include/mlir/IR/BuiltinAttributes.td
355–357

Maybe a concrete example? dims<1, 2, 3>

360

You should also document that embedding the attribute in an op's assembly format results in a different format [1, 2, 3] since the interaction with the builtin attribute parser/printer isn't obvious.

mlir/lib/Parser/AttributeParser.cpp
841–857

Also, can you separate out the lambda definition?

This revision now requires changes to proceed.Dec 30 2021, 9:05 AM
jpienaar added inline comments.Dec 30 2021, 9:18 AM
mlir/lib/Parser/TokenKinds.def
85

Do we really need a new keyword? This seems a very specific attribute and added to built-in (I'm not saying it isn't useful, just this is very privileged compared to just wanting a dense attribute with specialized storage)

mehdi_amini added inline comments.Dec 30 2021, 12:12 PM
mlir/lib/Parser/TokenKinds.def
85

The main question is how would you print it: #builtin.dims<1, 2 ,3>?

mehdi_amini marked 3 inline comments as done.

Add a trait to help for defining ArrayRef-like Attributes in general and use it for this new attribute, also address Jeff's comments.

Mogball accepted this revision.Dec 31 2021, 8:23 AM
Mogball added inline comments.
mlir/include/mlir/IR/AttributeSupport.h
280 ↗(On Diff #396751)

I'm not against having this trait, but is remapping literally everything necessary?

mlir/include/mlir/IR/BuiltinAttributes.td
364

Copy paste error?

370
This revision is now accepted and ready to land.Dec 31 2021, 8:23 AM
Mogball requested changes to this revision.Dec 31 2021, 8:26 AM

Some of the relevant changes in the Tosa patch need to be moved here

This revision now requires changes to proceed.Dec 31 2021, 8:26 AM

Backport changes the were in the child revision by mistake

mehdi_amini added inline comments.Dec 31 2021, 12:43 PM
mlir/include/mlir/IR/AttributeSupport.h
280 ↗(On Diff #396751)

If we don't forward the APIs, then when you can do something like for (int64_t dim : op.getDimensions()), you have to always do for (int64_t dim : op.getDimensions().getArray()) or cast explicitly: for (int64_t dim : ArrayRef<int64_t>(op.getDimensions()))

mehdi_amini marked 2 inline comments as done.

Fix copy/paste error in doc and fully qualified namespace

Mogball accepted this revision.Dec 31 2021, 12:50 PM
This revision is now accepted and ready to land.Dec 31 2021, 12:50 PM
rriddle requested changes to this revision.Dec 31 2021, 12:55 PM

Blocking until after holiday so I can take a look. I'm a bit apprehensive about how some of this is setup right now, namely why we need a specialized builtin attribute just for dimensions. Also the ArrayRefAttr API looks like a more general range mixin (likely living in ADT).

This revision now requires changes to proceed.Dec 31 2021, 12:55 PM

Rebase: remove all the ArrayRef methods after fixing the accessors on operands in D116466