[Mlir-commits] [mlir] [MLIR][Vector] Add Lowering for vector.step (PR #113655)

[Manupa Karunaratne]

manupak wrote:

Thanks @banach-space for taking another look..

> Can you link a specific example from that file that doesn't lower to other Vector Ops?

> Bare in mind that there are many examples in-tree that e.g. don't follow our coding guidelines. However, that doesn't mean that it's OK to violate them, it merely means that there's code that should be re-formatted ;-)

https://github.com/llvm/llvm-project/blob/340cd4e631d72d02cd79f9aad74d2a354abc977e/mlir/lib/Dialect/Vector/Transforms/LowerVectorContract.cpp#L1374-L1380

I did not mean to cite as "just because there are other code that does the same thing". What I wanted bring up, you would need `arith` dialect more inter-twined with other shaped type dialects such as `vector` and `tensor`. Therefore a dialect conversion that from `VectorToArith` did not make sense to me.

> A2 is better choice.

I can do A2.

> As for testing, I'd add this pattern to TestVectorScanLowering (and rename it as TestVectorToArithLowering). That would be the least intrusive and the most straightforward path for now.

May I ask why `vector.scan` is special to be combined with `vector.step` ? Only reason I see its rewrite pattern is named as "ScanToArithOps" but that seemed like a random choice. For e.g. `vector.mask` do use `arith` ops as well many other ops in LowerVector{}.cpp files. Most of the lowering patterns involve at least creating an `arith.constant`.  There are few others which involve a partial lowering to `arith` such as TestVectorGatherLowering.

Therefore, I personally feel its better to keep them seperated rather than combining two things together unless you have a much deeper reasoning why `vector.scan` and `vector.step` should be tested in a combined fashion.

> Having a dedicated TD op for such a small pattern feels like an overkill and we should probably find other patterns to group this one with. I don't really have a good suggestion just now, so I'd leave it as a TODO. 

This is what I meant as A2 isn;t it ? 
A2) include the rewriter in transform.structured.vectorize_children_and_apply_patterns



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