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

[Manupa Karunaratne]

manupak wrote:

Thanks all, I ll be working on addressing the specific comments.

Replying to higher-level comments from @banach-space,

> Do we? Is there a use case for this? I just want to make that we are not moving code for the sake of ... moving code around

Yes, I think @Groverkss is spot on the use case here. 
In a higher-level, `vector.step` carry the semantics of specialized constant which has `f(i) = i` property. This is immediately lost if 'fold' it away.  Therefore I d consider that as a lowering to lower level abstraction over a 'folder'.

> Re layering, transformations in "mlir/lib/Dialect/Vector/Transforms/LowerVector{.*}.cpp" lower from higher-level Vector Ops to lower-level Vector ops

Is this true ?
I can cite few examples that violates the notion that LowerVector*.cpp exclusively contains `vector` -> `vector` rewrites such as e.g. : LowerVectorContract. I suppose arith dialect is a bit special in notion that it inter mixes with most dialects. If we are to push for a seperation of dialects via a dialect conversion, I feel we will have to replicate most operations in `vector` (such as `vector.add`) .. Therefore, Im not fully following the case for a `VectorToArith` because its feels impossible `vector` to be practically used w/o `arith`.

> So, to me, there are actual benefits to keeping this as a folder.

Im not following the logical inference of the absense of  `VectorToArith` to a keep this as a folder. Can you help me understand this a bit more?
At least to me personally, folders should be proven always beneficial to have the transformation where here we are presenting a counter example for the folding of `vector.step`.

> Re changes in tests, we should avoid adding more patterns and TD Ops to the vectorizer tests that already use transform.structured.vectorize_children_and_apply_patterns (this TD Op should encapsulate everything that's needed).

> What happens if you don't add transform.apply_patterns.vector.step_to_arith to the tests?

This part I somewhat agree. 

So there was three options : 
A1) Introduce and use `transform.apply_patterns.vector.step_to_arith` to showcase expectation of tests are still met (more or less) using the lowering. -- I took this one but happy to change
A2) include the rewriter in `transform.structured.vectorize_children_and_apply_patterns`
A3) keep it as `vector.step` 

I can switch to A2 and A3. Having read again what `transform.structured.vectorize_children_and_apply_patterns` entails, I feel A2 is better choice. WDYT ?





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