Rebase

Rebase.

Rebase again.

Rebase

Awesome! Thanks!

Fix lit tests.

Fix bug.

I am unblocking given it will be iterated on. Happy to chat offline later

Probably needs to be into NamedOpConversion pass

Maybe we should file a bug to fix the issue with simplification

This seems strange to me. There is nothing that prevents a subsequent transformation to collapse the cast into the slice. Indeed it is probably better to do so.

Thanks!

I know this pattern exists in IREE, but it is a bit of a hack. The representation of the im2col as a linalg.generic doesnt work as well. In reality it is similar to a gather. If this is upstreamed, it might be worth doing this right and not use an unnecessarily higher-dimensional operation for representing the im2col. Maybe we can chat offline?

Sure. Just push the test. Thanks!

@Ding I tried to rebase this and land. Seems to fail tests.

I don't think so. I am AFK. I can merge when I get to my machine

I see why you made it a function pass, but I think it needs to be a pass on FunctionOpInterface. There are examples of this in IREE https://github.com/iree-org/iree/blob/55d0ffe7ea4ca5b58dd9197608151cc2f8c7521c/compiler/src/iree/compiler/Dialect/Flow/Transforms/Passes.td#L65 .

Have one comment here, more for my understanding of the nuances. Change itself looks good!

Address comments.

In D142480#4081907, @springerm wrote:

If a tensor.insert_slice can be hoisted, bufferization should be immune to that (I'd think it would even be better, but dont know the example that you have in mind). Hoisting entire then and else branches and ending up with a select seems to be a good thing in general.

Yes it is actually better for the bufferization — a flat structure is easier to bufferize than one with nested blocks; fewer things can go wrong.

But hoisting things also means that ops that were previously inside a branch and executed 50% of the time are now executed 100% of the time. (After bufferization, pure tensor ops have a side effect and therefore a cost.)

I think the core logic being fixed here is not something I worked on, so I dont have full context. Adding folks who worked on this as reviewers...

This is pretty cool! It makes CSE very powerful. Last time I edited, I tried to walk a fine line and take small steps. So thanks for cleaning all of these up! This is taking a big step. I think its OK. But I'll probably wait for others to chime in.

In D142480#4079206, @springerm wrote:

In D142480#4077817, @mravishankar wrote:

Then I am confused. If it is speculatable only, then you can just hoist.. Then CSE will handle the equivalence and replacing uses.... Not sure what I am missing....

What I meant:

%r = scf.if ... {
  "non_specutable_op"()
  ...
} else {
  "non_specutable_op"()
  ...
}

==> /* This transformation does not affect performance. */

"non_specutable_op"()
%r = scf.if ... {
  ...
} else {
  ...
}

Hoisting and CSE'ing "non_spectuable_op" from both branches is OK. Any op for that matter. Just hoisting from the "then" branch is not OK; that's only allowed if the op is pure and specutable.

My main concerns about "just hoisting" are performance implications. Should we hoist a tensor.insert_slice from an scf.if? That's a buffer copy after bufferization, so it can lead to a performance regression. We could go even further: Hoist the entire "then" and "else" bodies (assuming they are all pure+specutable ops; usually the case in tensor IR), so that only the terminators are left over; scf.if will become an arith.select. That's probably not desirable.

In D142480#4077799, @springerm wrote:

You are CSE-ing and hoisting... thats already within the preview of treating it as a speculatable op. Doing this on non-speculatable ops would be illegal AFAIK.

That’s why this pass is specific to scf.if. It does not hoist or CSE non-specutable ops from other ops. That should be legal.

In D142480#4077680, @springerm wrote:

In D142480#4077671, @mravishankar wrote:

Could we just use the hoisting part here. You can just hoist operations outside of if and let standard CSE common out the users.... THis is duplicating a lot of functionality

We could but this allows us to CSE irrespective of op spectuability and it has no effect on performance (because CSE’d ops are executed in either branch).

Could we just use the hoisting part here. You can just hoist operations outside of if and let standard CSE common out the users.... THis is duplicating a lot of functionality

This is very painful... Maybe we can just hold off on this until it is needed....

Unblocking, but have one comment remaining.

Thanks!

In D142205#4069971, @springerm wrote:

In D142205#4069807, @mravishankar wrote:

The pattern itself looks fine, but I think this has a dependency issue. There is already a Tensor -> Linalg dependence. This is adding a Linalg -> Tensor dependence.... That could be problematic... is there a way to do this as part of bufferization or something, or move these patterns to Linalg?

Note it is a dependency from TensorTransforms -> Linalg and we already have it without this change. The implementation is actually largely copied from Tensor/Transforms/BufferizableOpInterfaceImpl.cpp.

The pattern itself looks fine, but I think this has a dependency issue. There is already a Tensor -> Linalg dependence. This is adding a Linalg -> Tensor dependence.... That could be problematic... is there a way to do this as part of bufferization or something, or move these patterns to Linalg?

unblocking since there was a PSA. Thanks!

Looks good. THanks. Just a few nits.

Yeah, this is fine by me.

In D141804#4066465, @pifon2a wrote:

For instance developers often store models/IR in linalg format to be able to work in isolation of the front end. Those kind of changes forces them to regenerate models.

I think this is the only valid argument against landing this PR in this whole thread.

To start with, I see at minima design consistency with the general dialect design I think I saw upstream so far.

Thanks! This was the fix I had in mind.

In D141804#4058265, @gflegar wrote:

From my experience integrating MLIR into Google's internal repository, breakages from changes like these seem to be very frequent (e.g., the two already mentioned above), so I don't see why this change should be blocked while others get a pass. If we want to be more careful about API breakages, then we should first formalize this somehow and apply the same rules to everyone.
I never considered the printed version of MLIR stable, and I'm not sure that its stability should be the goal that prevents improvements to the printing format.
Regarding churn, we should not forget that not improving the printed format also causes churn every time someone is reading or modifying it.
I think that redundancy in IR causes more issues than it solves, so I would prefer landing this change.

In D141804#4055148, @mravishankar wrote:

This is an unnecessary change that just causes downstream churn. Not sure it is worth it

Looking at the previous discussion, I think that people's opinions differ on this one. However, I would like to touch on the phrasing of this comment. The very fact that someone spent the time to make this change means that they do think it is necessary. Thus, in my opinion, stating that it is unnecessary in such absolutist terms is disrespectful. The same point could have been achieved by saying something like "I disagree with this change because <your argumentation>". Even if you know Alex personally, and you're sure he wouldn't mind the statement, this is still a public forum where others (maybe potential future contributors) will read these discussions. I think we should be more careful in how we communicate, and make sure that this forum is an inclusive place where others' opinions are respected.

In D141804#4058011, @mehdi_amini wrote:

As a meta-point: I'm concerned about the push-back on improving things based on churn. The comment about users wanting stability can only be an argument about spending more time on design and being more thorough before adding anything to the codebase. However that means much less velocity and higher cost to experiment. So far this isn't the tradeoff we've been making in MLIR I believe.

In D141804#4057169, @herhut wrote:

I am in favor of landing this (but please ensure consensus before doing so). I have argued against changes before that were purely aesthetic but introduced a lot of churn (like the one in https://reviews.llvm.org/D133076). Ultimately it is a subjective decision. I have a different opinion in this case because the linalg dialect has a smaller usage scope compared to arith, so I am willing to accept more churn. Also, this change improves ergonomics when reading linalg IR, which we increasingly do and hence care about. This was also one of the motivations to introduce new operations to linalg like map.

This patch had a use-after-free issue. Fixed with https://reviews.llvm.org/D141869

In D141804#4057066, @springerm wrote:

I think this is worth cleaning this up. Small things like these can make it look like linalg is a second-class dialect where we don't care about code quality.

In D141804#4055864, @pifon2a wrote:

In D141804#4055148, @mravishankar wrote:

This is an unnecessary change that just causes downstream churn. Not sure it is worth it

It makes IR shorter and nicer. It is not a lot of churn, the named ops are not as widely used as linalg.generic. 90% of the tests were fixed by a simple sed command with s/ outs(\(.*\)) -> .*$/ outs(\1)/g.

The "churn" aspect is subjective. There were patches like https://github.com/llvm/llvm-project/commit/abc362a1077b9cb4186e3e53a616589c7fed4387, that were not blocked.

Address comments.