Page MenuHomePhabricator

[mlir] Remove types from attributes
ClosedPublic

Authored by Mogball on Jul 19 2022, 8:13 AM.

Details

Summary

This patch removes the type field from Attribute along with the
Attribute::getType accessor.

Going forward, this means that attributes in MLIR will no longer have
types as a first-class concept. This patch lays the groundwork to
incrementally remove or refactor code that relies on generic attributes
being typed. The immediate impact will be on attributes that rely on
Attribute containing a type, such as IntegerAttr,
DenseElementsAttr, and ml_program::ExternAttr, which will now need
to define a type parameter on their storage classes. This will save
memory as all other attribute kinds will no longer contain a type.

Moreover, it will not be possible to generically query the type of an
attribute directly. This patch provides an attribute interface
TypedAttr that implements only one method, getType, which can be
used to generically query the types of attributes that implement the
interface. This interface can be used to retain the concept of a "typed
attribute". The ODS-generated accessor for a type parameter
automatically implements this method.

Next steps will be to refactor the assembly formats of certain operations
that rely on parseAttribute(type) and printAttributeWithoutType to
remove special handling of type elision until type can be removed from
the dialect parsing hook entirely; and incrementally remove uses of
TypedAttr.

Diff Detail

Event Timeline

Mogball created this revision.Jul 19 2022, 8:13 AM
Mogball requested review of this revision.Jul 19 2022, 8:13 AM
Mogball edited the summary of this revision. (Show Details)Jul 19 2022, 8:21 AM
Mogball edited the summary of this revision. (Show Details)
mehdi_amini added inline comments.Jul 19 2022, 8:45 AM
mlir/include/mlir/IR/AttrTypeBase.td
401

Can you document the typeBuilder parameter as well as this logic?

lattner accepted this revision.Jul 19 2022, 9:15 AM

Oh nice, TypedAttr is a great idea. I'm very +1 on this direction. Thank you Jeff!

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
2418

If the types are the same, the attributes should unique to be the same thing, can you just change this to something like:
if (re != im || !re) ?

mlir/lib/Parser/DialectSymbolParser.cpp
253 ↗(On Diff #445829)

Super nit but you can use:

if (auto typedAttr = attr.dyn_cast_or_null<TypedAttr>()) {

This revision is now accepted and ready to land.Jul 19 2022, 9:15 AM
rriddle accepted this revision.Jul 19 2022, 9:25 AM

No doc updates were necessary?

mlir/include/mlir/IR/BuiltinAttributeInterfaces.td
160
483–484
Mogball updated this revision to Diff 445891.Jul 19 2022, 11:20 AM
Mogball marked 5 inline comments as done.

Review comments and update docs a little.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
2418

TypedAttr is an interface so the underlying attributes can still be different, e.g. [-1.0 : f32, 1.0 : f32] is valid

Oh right, of course, thx.

jpienaar accepted this revision.Jul 19 2022, 12:55 PM
jpienaar added a subscriber: jpienaar.

Nice, thanks!

mlir/include/mlir/IR/BuiltinAttributeInterfaces.td
478

With a type?

mlir/include/mlir/IR/BuiltinAttributes.td
199–202

Could you add what the shaped type represents?

mlir/lib/IR/BuiltinAttributes.cpp
705–707

OOC why is this called tblgenKey?

1003

Shouldn't this be in the attributes verify somewhere?

jloser added a subscriber: jloser.Jul 19 2022, 4:01 PM
jloser added inline comments.
mlir/docs/AttributesAndTypes.md
716

Super nit: s/Parameterk/Parameter

Mogball added inline comments.Jul 22 2022, 12:22 PM
mlir/lib/IR/BuiltinAttributes.cpp
1003

This might be legacy code. There are a lot of redundant asserts around here

Mogball updated this revision to Diff 448891.Jul 31 2022, 3:29 PM
Mogball marked 5 inline comments as done.

review comments

This revision was landed with ongoing or failed builds.Jul 31 2022, 5:01 PM
This revision was automatically updated to reflect the committed changes.

huzzah, congrats Jeff!

QQ: Should a TypeAttr also implements the TypeAttrInterface. The fact that is not is causing some problems with IREE integration.

I don’t get why it should? What would be the type of a type attribute? Returning the holder type does not seems consistent to me: I see TypedAttr interface to indicate the type of the data wrapped by the attribute, we would need some sort of meta-type system.
What is the use case for IREE?

I see your point. It makes sense to me. It looks like we had some code that was resilient to null types and I assumed operands should be converted to TypedAttr, which doesn't seem to be the case. Thanks for clarifying!