[RFC] Removing OpaqueElementsAttr

OpaqueElementsAttr is technical debt from the very early days of MLIR, before I transformed ElementsAttr into an interface and when it was more difficult for dialects to define their own attributes. At present this attribute has no real value aside from being a convenient mechanism for eliding other ElementsAttr. IMHO we should delete it. The only user (that I know of) is TensorFlow, but that is merely because it has used this attribute since the beginning (i.e. years ago). Users today wouldn’t use OpaqueElementsAttr, and instead would/should go with the more ergonomic, maintainable, and usable approach of defining their own.

This thing doesn’t pay for itself, and I struggle to see a path to where it does (given ElementsAttr is an interface). Does anybody have serious concerns about getting rid of it? I’ve uploaded a patch here: ⚙ D129917 [mlir] Remove OpaqueElementsAttr

– River

1 Like

None. Thank you for cleaning up this early days debt.

Nice – love that it now prints elided attrs as splats so the IR is actually “real” and can be run through flows. :smiley:

It’s interesting because I see this exactly as a regression right now (and I mean, if it was desired behavior it could have been the case from the beginning)

Yes, it is a fairly “dangerous” thing. Also splats are not the same as dense elements: various code paths do micro-optimize on that.

(but agreed with the convenience factor: just wary that this does actually change the behavior of the compiler)

This is a big sticking point for me though, given that by changing DenseElementsAttr to something else we are already changing the behavior of the compiler (and potentially creating verification errors). No one should expect elided elements to work the same way as non-elided. The big thing here I guess, is what the actual desired behavior on elision is. We are currently making tradeoffs of what is acceptable to be broken and what isn’t, but we shouldn’t pretend that any solution we come up with will be correct in all forms.

– River

I guess I’m more just calling out that before, the IR was syntactically valid but in all likelihood, unless if manipulating it further, it wouldn’t actually be usable in place of the original. From one perspective, that is a feature. From the other perspective, it is sure convenient that it now most likely does “something”, but we are left with that “something” being undefined and a potential footgun (i.e. I expect that folks will now go out and use this flag to produce versions of IR for some kind of benchmarking in cases where they “don’t care about the values”).

Just something to keep in mind. I’m not sure what the answer is or that there is a real issue.