Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
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.
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.
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.
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!
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.
For me it is highly confusing to read "outputs" and associate this to immutable objects (tensors). Seeing how people try to form a mental model around this for the last 2 years, I really want an alternative spelling that won't bring this confusing and respect the immutability of tensors.
Seems that inits is where we converged on Discourse, but if you want to propose other spelling (that don't imply "writing to a tensor") please do!