Page MenuHomePhabricator

[mlir] Refactor ElementsAttr into an AttrInterface
ClosedPublic

Authored by rriddle on Sep 2 2021, 1:31 PM.

Details

Summary

This revision refactors ElementsAttr into an Attribute Interface.
This enables a common interface with which to interact with
element attributes, without needing to modify the builtin
dialect. It also removes a majority (if not all?) of the need for
the current OpaqueElementsAttr, which was originally intended as
a way to opaquely represent data that was not representable by
the other builtin constructs.

The new ElementsAttr interface not only allows for users to
natively represent their data in the way that best suits them,
it also allows for efficient opaque access and iteration of the
underlying data. Attributes using the ElementsAttr interface
can directly expose support for interacting with the held
elements using any C++ data type they claim to support. For
example, DenseIntOrFpElementsAttr supports iteration using
various native C++ integer/float data types, as well as
APInt/APFloat, and more. ElementsAttr instances that refer to
DenseIntOrFpElementsAttr can use all of these data types for
iteration:

c++
DenseIntOrFpElementsAttr intElementsAttr = ...;

ElementsAttr attr = intElementsAttr;
for (uint64_t value : attr.getValues<uint64_t>())
  ...;
for (APInt value : attr.getValues<APInt>())
  ...;
for (IntegerAttr value : attr.getValues<IntegerAttr>())
  ...;

ElementsAttr also supports failable range/iterator access,
allowing for selective code paths depending on data type
support:

c++
ElementsAttr attr = ...;
if (auto range = attr.tryGetValues<uint64_t>()) {
  for (uint64_t value : *range)
    ...;
}

Depends On D104173

Diff Detail

Event Timeline

rriddle created this revision.Sep 2 2021, 1:31 PM
rriddle requested review of this revision.Sep 2 2021, 1:31 PM
jpienaar accepted this revision.Sep 3 2021, 2:38 PM

Nice, thanks! Any overhead here to this change from the original accessors?

mlir/include/mlir/IR/BuiltinAttributeInterfaces.h
2

nit: I'd drop MLIR to get more chars if needed (seems redundant given where it is)

32

s/whose/and/ ?

33

Does this mean it is up to user to cache as needed?

58

Same

164

For class size, does it make difference if we have the bools after the union?

184

OOC does this need to be a doxygen comment or just a plain comment?

mlir/include/mlir/IR/BuiltinAttributeInterfaces.td
32

implementing ?

37

can or they should? If I don't implement these, what do I get?

178

Shall we consistently use Returns or Return?

187

Nit: this is too literally the check, how about Returns if this attribute hold no elements ?

216

1-dimensional ? (I first read it as (1) (dimensional), rather than (1 dimensional))

mlir/include/mlir/IR/BuiltinAttributes.td
170

Do we need ::mlir::DenseElementsAttr here?

185

OOC why no signed complex numbers?

mlir/test/IR/elements-attr-interface.mlir
3

Could you document test?

22

Newline?

mlir/test/lib/Dialect/Test/TestAttrDefs.td
78

This is nice in code, how would these look like post doxygen?

This revision is now accepted and ready to land.Sep 3 2021, 2:38 PM
rriddle updated this revision to Diff 373735.Mon, Sep 20, 3:58 PM
rriddle marked 16 inline comments as done.

Update

Nice, thanks! Any overhead here to this change from the original accessors?

For contiguous ranges, performance is essentially the same as before. For non-contiguous ranges, there is some additional overhead (given the virtual call) but in testing the performance was still quite comparable to before.

mlir/include/mlir/IR/BuiltinAttributeInterfaces.h
33

Essentially. non-contiguous ranges can take many forms, and this sentence is intended to convey that no guarantees are made on the lifetime of values or their contiguity. The derived attributes will need to make their own trade offs on computational complexity and caching (e.g. DenseElementsAttr doesn't cache the Attribute values, and instead constructs them on demand).

164

Not really in this case, the class is going to be word-aligned so we should end up with the same amount of padding wherever the bools are placed.

184

Just dropped to a regular comment.

mlir/include/mlir/IR/BuiltinAttributeInterfaces.td
37

Can (but switched the wording to may). If an attribute doesn't provide iterable types, like in the case of OpaqueElementsAttr, they just don't support iteration of any kind. The interface still provides other various utility methods, and also allows for user defined ElementsAttr to be used with operations that accept ElementsAttr. An ElementsAttr is not required to support iteration of any kind, it's up to the user. We could enforce it, but that could be an abstraction built on top of ElementsAttr (e.g. something like IterableElementsAttr/IterableElementsAttr<Attribute>/etc.).

mlir/include/mlir/IR/BuiltinAttributes.td
170

No, full namespace resolution is only necessary in generic tablegen code. (This code block is always placed in mlir::)

185

(I forgot, thanks!)

mlir/test/lib/Dialect/Test/TestAttrDefs.td
78

Not entirely sure, dropped down to normal comments for now. (I should check our doxygen output at some point).

This revision was landed with ongoing or failed builds.Mon, Sep 20, 6:58 PM
This revision was automatically updated to reflect the committed changes.