This attribute is technical debt from the early stages of MLIR, before
ElementsAttr was an interface and when it was more difficult for
dialects to define their own types of attributes. At present it isn't
used at all in tree (aside from being convenient for eliding other
ElementsAttr), and has had little to no evolution in the past three years.
Details
- Reviewers
jpienaar mehdi_amini - Commits
- rG40abd7ea641b: [mlir] Remove OpaqueElementsAttr
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Nice clean-up, keyword freed up too.
mlir/test/CAPI/ir.c | ||
---|---|---|
476 | Anyway to still retain marker that elided? (I played around in a follow up of my outline constants patch where I used alias mechanism ... Mmm we could have an ElidedAttribute that implements the ElementsAttr but has only shape, just concerned about not being able to differentiate between a real and elided value. |
mlir/test/CAPI/ir.c | ||
---|---|---|
476 | I agree that it is a bit of a loss from the previous behavior in some aspects, but I'm -1 to having an attribute just for elision. I thought about this a bit when drafting this commit, but it just isn't the right direction. For one, the attribute would have no purpose other than for a special textual mode (which is weird in and of itself), and I don't think we'd add something special for any of the other types of attributes if we wanted to elide them. Also, the previous behavior wrongly assumed that all uses of DenseElementsAttr/SparseElementsAttr could be changed to OpaqueElementsAttr. We have just gotten lucky that the cases we have generally elided were okay with any ElementsAttr attribute. |
I am concerned about turning elided constant into splat right now: this breaks reproducibility in a way that is too subtle and error prone as far as I understand it.
No, but this is not a strong enough justification to warrant keeping it. I don't think we should have an attribute solely for elision, so we don't have a huge number of options right now.
I don't necessarily disagree with you that we may not want to keep an attribute for this purpose (even though that's debatable). I disagree however in that the motivation to remove it is enough to degrade the elision feature in this way right now: I don't think the new behavior is OK.
Should we drop the ability to run elided IR then? What do you have in mind? I'm strongly against keeping this just for elision, especially because it's not even valid in all cases anyways.
The only way I really see keeping this behavior (without using splat) is by using https://reviews.llvm.org/D130022 (builtin ElementsAttr backed by resources). That still has the problem though that the IR isn't guaranteed to be valid if an op actually expects DenseElementsAttr/SparseElementsAttr/etc.
Anyway to still retain marker that elided? (I played around in a follow up of my outline constants patch where I used alias mechanism ... Mmm we could have an ElidedAttribute that implements the ElementsAttr but has only shape, just concerned about not being able to differentiate between a real and elided value.