[PATCH] D109190: [mlir] Refactor ElementsAttr into an AttrInterface

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

In D109190#2983216 <https://reviews.llvm.org/D109190#2983216>, @jpienaar wrote:

> 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.



================
Comment at: mlir/include/mlir/IR/BuiltinAttributeInterfaces.h:32
+/// non-contiguous range implies no contiguity, whose elements may even be
+/// materialized when indexing, such as the case for a mapped_range.
+struct ElementsAttrIndexer {
----------------
jpienaar wrote:
> Does this mean it is up to user to cache as needed?
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).


================
Comment at: mlir/include/mlir/IR/BuiltinAttributeInterfaces.h:163
+  /// A boolean indicating if this range is contiguous or not.
+  bool isContiguous;
+  /// A boolean indicating if this range is a splat.
----------------
jpienaar wrote:
> For class size, does it make difference if we have the bools after the union?
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.


================
Comment at: mlir/include/mlir/IR/BuiltinAttributeInterfaces.h:183
+
+  /// Boilerplate iterator methods.
+  ptrdiff_t operator-(const ElementsAttrIterator &rhs) const {
----------------
jpienaar wrote:
> OOC does this need to be a doxygen comment or just a plain comment?
Just dropped to a regular comment.


================
Comment at: mlir/include/mlir/IR/BuiltinAttributeInterfaces.td:36
+
+    An attribute can define the set of iterable types by providing a definition
+    of tuples `ContiguousIterableTypesT` and/or `NonContiguousIterableTypesT`.
----------------
jpienaar wrote:
> can or they should? If I don't implement these, what do I get?
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.).


================
Comment at: mlir/include/mlir/IR/BuiltinAttributes.td:170
   let extraClassDeclaration = [{
+    using DenseElementsAttr::empty;
+    using DenseElementsAttr::getFlatValue;
----------------
jpienaar wrote:
> Do we need ::mlir::DenseElementsAttr here?
No, full namespace resolution is only necessary in generic tablegen code. (This code block is always placed in mlir::)


================
Comment at: mlir/include/mlir/IR/BuiltinAttributes.td:185
+      std::complex<uint8_t>, std::complex<uint16_t>, std::complex<uint32_t>,
+      std::complex<uint64_t>,
+      // Float types.
----------------
jpienaar wrote:
> OOC why no signed complex numbers?
(I forgot, thanks!)


================
Comment at: mlir/test/lib/Dialect/Test/TestAttrDefs.td:78
+    }
+    /// * Attribute
+    auto value_begin_impl(OverloadToken<mlir::Attribute>) const {
----------------
jpienaar wrote:
> This is nice in code, how would these look like post doxygen?
Not entirely sure, dropped down to normal comments for now. (I should check our doxygen output at some point).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109190/new/

https://reviews.llvm.org/D109190