[Mlir-commits] [mlir] [mli][vector] canonicalize vector.from_elements from ascending extracts (PR #139819)

[James Newling]

newling wrote:

@banach-space thanks for the thorough review! I'll implement your suggestions soon. 

> I am unsure about making this a canonicalization and hopefully somebody else could chime in as well. Basically, the IR becomes much simpler (+1), but "data movement" Ops are replaced with an Op for which:
> 
> > It is currently assumed that this operation does not require moving data"
> 
> This makes it a bit tricky for me. In my view, as long as this pattern composes nicely with other transformations (e.g. we don't re-materialise the data movement Ops to "lower" `vector.shape_cast`), it should be safe/fine to include it as a canonicalization.

I share this concern. Specifically, if the lowering of `vector.shape_cast` were to create a `vector.from_elements`, then canonicalization would un-lower `shape_cast`. It currently doesn't lower like this, but it doesn't seem unreasonable.  

I could take the conservative approach of making this a folder along the lines of https://github.com/llvm/llvm-project/pull/135841#issue-2997350048, so effectively converting to `vector.shape_cast` only if we know it will be absorbed immediately into another `vector.shape_cast`. One concern here is that this pattern is O(num elements in vector) and so not the fastest, and folding generally happens more often than canonicalizing.  Another concern is that it goes against the idea suggested in https://github.com/llvm/llvm-project/pull/138777 of canonicalizing transpose/broadcast/extract to shape_cast where possible. 

My personal preference would be to not avoid creating `shape_cast`. And to reconsider its lowering, as you suggested [here](https://github.com/llvm/llvm-project/pull/138777#issuecomment-2876352256). Perhaps shape_cast should be treated as the bottom rung on the lowering ladder, an op which gets converted directly to llvm/other (in the case where not all shape_casts have cancelled). 

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