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

[Hugo Trachino]

================
@@ -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> {
----------------
nujaa wrote:

Makes sense to me, it deserves refactoring and standardization. I could have done better on commenting and naming.

> add a big bold comment to document what's being tested here, something similar to what's on L94-L101

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

> add no_transpose to the test functions names that you added.

I think this quite goes against the idea of having a permutation specific file. as you mention there :

> there should be no non-permutation examples in this file, right? To make things even more confusing, these patterns are also tested in:

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.

>  these patterns are also tested in:

I would not mind about `transform-vector.mlir` as it is a full pipeline test. The goal is to show and test a complete lowering which makes sense. A bit like the e-2-e examples you added for SME.
For vector-transfer-to-vector-load-store.mlir , I think this file deserves improvements too. (such as the repetition of same transform sequences. ) The concept is to combine it with lower_transfer to test the complete lowering of xfer ops. it is not very _**unit**_ test like but it shows how to properly lower xfer ops which is a benefit. Mixed feelings. I think there should not be `transfer_permutation_patterns` specific tests here, but I find sensible to test this combination of patterns. We should maybe ask @ftynse .

> it's bit confusing that TransferOpReduceRank is added to populateVectorTransferPermutationMapLoweringPatterns:

Indeed, but after all it is a lowering based on the permutation map of a TransferRead. Hence, I think than rather removing `TransferOpReduceRank`,  the ideal solution is to rename `TransferOpReduceRank`  to make clear it matches patterns in the permutation map or renaming `populateVectorTransferPermutationMapLoweringPatterns` to reflect it is not mandatory a permutation. This set of patterns is useful as a whole for the community as a lowering pass.

I would also add something. As @hanhanW  mentioned in https://github.com/llvm/llvm-project/pull/93664#pullrequestreview-2089507138  we could get rid of  `/// ``` and standardize it.
` in Doxygen descriptions for improved readability.

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