This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Remove OpaqueElementsAttr
ClosedPublic

Authored by rriddle on Jul 15 2022, 8:08 PM.

Details

Summary

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.

Diff Detail

Event Timeline

rriddle created this revision.Jul 15 2022, 8:08 PM
rriddle requested review of this revision.Jul 15 2022, 8:08 PM
jpienaar accepted this revision.Jul 15 2022, 8:47 PM

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.

This revision is now accepted and ready to land.Jul 15 2022, 8:47 PM
rriddle added inline comments.Jul 16 2022, 9:57 PM
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.

mehdi_amini requested changes to this revision.Jul 18 2022, 5:57 AM

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.

This revision now requires changes to proceed.Jul 18 2022, 5:57 AM

Have you already thought of alternatives for it?

rriddle added a comment.EditedJul 18 2022, 6:04 AM

Have you already thought of alternatives for 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.

Have you already thought of alternatives for 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.

Have you already thought of alternatives for 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.

rriddle added a comment.EditedJul 18 2022, 9:01 AM

Have you already thought of alternatives for 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.

rriddle requested review of this revision.Aug 1 2022, 1:45 PM
rriddle updated this revision to Diff 449114.

Updated to use the new resource attr for elision.

mehdi_amini accepted this revision.Aug 1 2022, 2:04 PM
This revision is now accepted and ready to land.Aug 1 2022, 2:04 PM
This revision was automatically updated to reflect the committed changes.
mlir/include/mlir/IR/BuiltinAttributes.td