[WIP] change dense attr representation to bytes #3531
Conversation
| if isinstance(element_type, IndexType): | ||
| # store IndexType as long long in bytes object | ||
| format_str = "{}q" |
There was a problem hiding this comment.
I think we might want to remove support for Index before this PR, as the size of index is a function of the target, not the host computer.
| | RankedStructure[AnyFloat] | ||
| ] | ||
| data: ParameterDef[ArrayAttr[AnyIntegerAttr] | ArrayAttr[AnyFloatAttr]] | ||
| type: ParameterDef[RankedStructure[IntegerType | IndexType | AnyFloat]] |
There was a problem hiding this comment.
this seems to be an independent change?
| elif isinstance(element_type, IntegerType): | ||
| if element_type.size == 1: | ||
| # 1 byte signed char | ||
| format_str = "{}b" |
There was a problem hiding this comment.
it would be great to share code with the ptr.py file, if possible, potentially moving shared utilities to a third location.
| ] | ||
| data: ParameterDef[ArrayAttr[AnyIntegerAttr] | ArrayAttr[AnyFloatAttr]] | ||
| type: ParameterDef[RankedStructure[IntegerType | IndexType | AnyFloat]] | ||
| data: ParameterDef[BytesAttr] |
There was a problem hiding this comment.
I would recommend first migrating all users of this property to another property/getter that returns a sequence of AnyIntegerAttr or a sequence of AnyFloatAttr, to minimise the diff in other files (probably also a property/getter that returns a sequence of ints or a sequence of floats)
| | RankedStructure[IndexType] | ||
| | RankedStructure[AnyFloat] | ||
| ] | ||
| data: ParameterDef[ArrayAttr[AnyIntegerAttr] | ArrayAttr[AnyFloatAttr]] |
There was a problem hiding this comment.
Wouldn't it make sense to migrate ArrayAttr to be bytes-based instead of DenseIntOrFPElementsAttr, BTW?
There was a problem hiding this comment.
I'm not 100% sure of this BTW, but would be good to play with it and maybe chat about findings. It would be nice to have some sort of generic way to have a dense array of things, but maybe that should just be this class, and array should be left alone to explicitly be a tuple of attributes and nothing more.
There was a problem hiding this comment.
I feel like it would be better to keep them separate. ArrayAttr will always hold a new Attribute object for every one of its elements, and converting those to a dense array is not trivial (for example, ArrayAttr of AffineMapAttrs)
|
thank you for the comments! I'll first do some more incremental PRs before making the switch to |
No description provided.