Page MenuHomePhabricator

[mlir] Rename `outs` to `inits` for linalg ops.
AbandonedPublic

Authored by pifon2a on Oct 27 2022, 12:06 PM.

Diff Detail

Event Timeline

pifon2a created this revision.Oct 27 2022, 12:06 PM
pifon2a requested review of this revision.Oct 27 2022, 12:06 PM

LGTM for the sparse part, since it is a mechanical change. But it will have impact on many out-of-tree projects using the linalg syntax.
Was there a discourse posting with a heads up on this?

cota added a comment.Oct 27 2022, 1:14 PM

Can you please provide a rationale for the rename in the commit message? Or a link to a previous discussion on this topic. Thanks!

This may be contentious and not worth the churn.. not all outs are technically inits and vice versa.

readonly, writeonly, readwrite is the verbose spelling for ins, outs, inits (we even used to have getter/setter for the 3 categories at some point).

Not opposed to the change but if you have a better way to look at this that made you conclude that "init" is unambiguously better thatn "out", I'd love to understand it.

This may be contentious and not worth the churn.. not all outs are technically inits and vice versa.

readonly, writeonly, readwrite is the verbose spelling for ins, outs, inits (we even used to have getter/setter for the 3 categories at some point).

Not opposed to the change but if you have a better way to look at this that made you conclude that "init" is unambiguously better thatn "out", I'd love to understand it.

Second this. I have seen lots of people confused about outs and somehow saying init clarifies things when looking at Linalg on tensors.... but this might be fixable with documentation. The churn is not desirable though. It will just be a nuisance for downstream users.

pifon2a added a comment.EditedOct 28 2022, 2:03 AM

This may be contentious and not worth the churn.. not all outs are technically inits and vice versa.

readonly, writeonly, readwrite is the verbose spelling for ins, outs, inits (we even used to have getter/setter for the 3 categories at some point).

Not opposed to the change but if you have a better way to look at this that made you conclude that "init" is unambiguously better thatn "out", I'd love to understand it.

When we are in tensor world, isn't it always an "init" technically? even if it is a "tensor.empty".

@cota, @aartbik It was discussed in this RFC https://discourse.llvm.org/t/rfc-interface-for-destination-style-ops/64056/15?u=pifon2a

Actually it is a very simple sed command that replaces outs to inits in .mlir files. A much harder change is to rename the methods in DPS interface, which I have to do anyway because of name collisions: after mlir migrated to camel-case getters, there are conflicts between getInputs generated by tablegen and getInputs in the DPS interface. Since I am currently at it, I suggest we rename it to inits.

@cota, @aartbik It was discussed in this RFC https://discourse.llvm.org/t/rfc-interface-for-destination-style-ops/64056/15?u=pifon2a

Oh cool, I lost track of the fact consensus had been reached, fine with me then!

Thanks. Please add this link in the description on this revision. Also probably a good idea to "reply to" the discourse discussion with a note that this is happening
(last post was a while back ;-). But other than that LGTM as before.

pifon2a edited the summary of this revision. (Show Details)Oct 28 2022, 12:10 PM

Thanks for proposing this change, Alex! I agree that we should pay attention to using descriptive and precise names. Unfortunately, I've been digesting the getDPSInits() changes and I'm not sure this is going in the right direction. We now have code snippets that are somewhat confusing. For instance:

Process operation *outputs* doing X and Y.
... = getDPSInits();

I have spent some time scratching my head thinking... "hmm, where are the outputs in this code?". With this I'm not suggesting that we go and change the comments but that the name seems to be a bit counterintuitive. I haven't been following the discussion about why this change is needed but hopefully we can reconsider a name that is more intuitive like outputs, inouts, etc. Same for getDPSInits. It's true that in DPS outputs have an initial value but the output aspect of it has much more weight in what they represent, IMO.

Hopefully that helps!

@dcaballe

Hi Diego. The whole reason for this change is the discussion in https://discourse.llvm.org/t/rfc-interface-for-destination-style-ops/64056/15?u=pifon2a where people found "outputs" confusing :). I don't care about naming at all. I just want the getters to be consistent with the IR that we print.

pifon2a abandoned this revision.Fri, Jan 13, 5:34 AM