[Mlir-commits] [mlir] [mlir][vector] Sink vector.extract/splat into load/store ops (PR #134389)

[Diego Caballero]

================
@@ -161,6 +161,20 @@ void populateVectorTransferCollapseInnerMostContiguousDimsPatterns(
 void populateSinkVectorOpsPatterns(RewritePatternSet &patterns,
                                    PatternBenefit benefit = 1);
 
+/// Patterns that remove redundant Vector Ops by merging them with load/store
+/// ops
+/// ```
+/// vector.load %arg0[%arg1] : memref<?xf32>, vector<4xf32>
+/// vector.extract %0[1] : f32 from vector<4xf32>
+/// ```
+/// Gets converted to:
+/// ```
+/// %c1 = arith.constant 1 : index
+/// %0 = arith.addi %arg1, %c1 overflow<nsw> : index
+/// %1 = memref.load %arg0[%0] : memref<?xf32>
----------------
dcaballe wrote:

I think some specific examples would help clarify.

> Increasing the granularity of memory accesses may cause you not to be able to use wider load/store instructions,

I totally agree with this point and that's why I mentioned we should be making emphasis on the one element only. However, this pattern is constrained to using/extracting only one element, right? 

> This may also change the semantics of load instructions that support OOB behavior -- you can turn an OOB access into an in-bounds access.

Could you help me understand this? We should be able to remove any load operation that is dead, regardless of whether it's in-bounds or OOB, right? What makes this case different?

> Also vector.extract ... [5] : vector<8xi1>. Applying the pattern in this case means loses byte alignment which also doesn't seem like a good fit for a canonicalization to me.

What do you mean by losing byte alignment? We have to load byte aligned... 
Hmm... I'm now wondering if the transformation works for sub-byte types. Looking at an example would be very helpful.

Hopefully we can address these points before landing, thanks!

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