[Mlir-commits] [mlir] [mlir][vector] Add vector.transpose with unit-dim to vector.shape_cast pattern (PR #72105)

[Mehdi Amini]

joker-eph wrote:

Supporting SPIR-V is critical, and nobody wants to break it of course.

> It is really something that cannot be handled in SPIR-V and as such should not be on the default path.

I'm confused by this comment considering that the doc for the op is specified as:

> It is currently assumed that this operation does not require moving data, and that it will be folded away before lowering vector operations.

If the expectation is that this is basically a "no-op", it would be great to explain a bit more the SPIR-V issue?

It's not clear to me that the canonicalization is incorrect here, and it may instead be a missing lowering for the SPIR-V path?
Because fundamentally if you can support:

>   %0 = vector.transpose %arg0, [1, 0] : vector<4x1xf32> to vector<1x4xf32>

Then I quite don't understand why you cannot possibly support:

> %0 = vector.shape_cast %arg0 : vector<4x1xf32> to vector<1x4xf32>




On the process side:

> Sorry for being the "blocker" on moving forward--in my mind it's actually common in LLVM/MLIR land to revert if something causes an issue.  [...]

It is a bit more nuanced than that: we revert if we believe that there is a fundamental issue with the commit (design or other), or if we have confidence that there is a bug (in MLIR, not we're "exposing a bug in your downstream project"). Of course we can't have tests for every possible situation ahead of time, but the expectation on bug finding downstream is that promptly after reverting an upstream test is added to cover the missing situation.
If we have concerned on the design, it's perfectly fine to revert fast, discuss, and re-land as-is if things are cleared up! 


https://github.com/llvm/llvm-project/pull/72105