[Mlir-commits] [mlir] [MLIR][Vector] Implement TransferOpReduceRank as MaskableOpRewritePattern (PR #92426)

[Andrzej Warzyński]

================
@@ -187,3 +187,49 @@ module attributes {transform.with_named_sequence} {
     transform.yield
   }
 }
+
+// -----
+
+
+//       CHECK:   #[[MAP:.*]] = affine_map<(d0, d1, d2, d3) -> (d1, 0, d3)>
+//       CHECK:   func.func @transfer_read_reduce_rank_scalable(
+//  CHECK-SAME:       %[[ARG_0:.*]]: memref<?x?x?x?xf32>) -> vector<8x[4]x2x3xf32> {
+//       CHECK:     %[[C0:.*]] = arith.constant 0 : index
+//       CHECK:     %[[TFR:.*]] = vector.transfer_read %arg0[%[[C0]], %[[C0]], %[[C0]], %[[C0]]]{{.*}} permutation_map = #[[MAP]]} : memref<?x?x?x?xf32>, vector<[4]x2x3xf32>
+//       CHECK:     %[[BC:.*]] = vector.broadcast %[[TFR]] : vector<[4]x2x3xf32> to vector<8x[4]x2x3xf32>
+//       CHECK:     return %[[BC]] : vector<8x[4]x2x3xf32>
+func.func @transfer_read_reduce_rank_scalable(%mem: memref<?x?x?x?xf32>) -> vector<8x[4]x2x3xf32> {
----------------
banach-space wrote:

> But I think we should rather emphasize on the fact it is not a permutation lowering but a lowering based on the permutation map . Hence the no_transpose answer works.

That's spot-on and something that I missed. In fact, this is documented here:
https://github.com/llvm/llvm-project/blob/77db8b08c8b186c2625f8dfb26bb976561b43c4c/mlir/include/mlir/Dialect/Vector/Transforms/LoweringPatterns.h#L175-L228

With this in mind, please ignore my comments under this heading:
> Note on populateVectorTransferPermutationMapLoweringPatterns

In fact, sounds like [vector-transfer-permutation-lowering.mlir](https://github.com/llvm/llvm-project/pull/92426/files/12f68ff1983dcbd3e3ae5effb916f853f512600a#diff-87b7cedb5d39b7a7598c7dcf29c95ee7cf6522f85163c3044d23e19f1e6f232c) is the right file for all patterns under `populateVectorTransferPermutationMapLoweringPatterns`.

Going back to the problem at hand ... This

> Makes sense to me, it deserves refactoring and standardization.

and this:

> Alternatively, separating tests with a big bold comment by pattern they are actually testing from the set could make sense.

First step in this direction: https://github.com/llvm/llvm-project/pull/95529. Let me know if that makes sense and I will prepare more tests.

> I could have done better on commenting and naming.

Not your fault, it's hard to navigate this ATM (this is also why it takes me ages to review things). Clearly our test coverage is not great either. In general, Vector needs some TLC 😅 I really appreciate you helping us here 🙏🏻 

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