[Mlir-commits] [mlir] [mlir][vector] Improve flattening vector.transfer_write ops. (PR #94051)

[Andrzej Warzyński]

================
@@ -471,25 +471,27 @@ func.func @regression_non_contiguous_dim_read(%subview : memref<1x3x3x2xf32, str
 }
 
 //       CHECK:  #[[$MAP:.+]] = affine_map<()[s0] -> (s0 * 2)>
-// CHECK-LABEL:    func.func @regression_non_contiguous_dim_read(
-//       CHECK:      %[[COLLAPSE:.+]] = memref.collapse_shape %{{.*}} {{\[}}[0], [1], [2, 3]] : memref<1x3x3x2xf32, strided<[40, 10, 2, 1], offset: ?>> into memref<1x3x6xf32, strided<[40, 10, 1], offset: ?>>
-//       CHECK:     %[[APPLY:.*]] = affine.apply #[[$MAP]]()
+// CHECK-LABEL:  func.func @regression_non_contiguous_dim_read(
+//       CHECK:    %[[COLLAPSE:.+]] = memref.collapse_shape %{{.*}} {{\[}}[0], [1], [2, 3]] : memref<1x3x3x2xf32, strided<[40, 10, 2, 1], offset: ?>> into memref<1x3x6xf32, strided<[40, 10, 1], offset: ?>>
+//       CHECK:    %[[APPLY:.*]] = affine.apply #[[$MAP]]()
 
 // CHECK-128B-LABEL: func @regression_non_contiguous_dim_read(
 //       CHECK-128B:   memref.collapse_shape
 
 // -----
 
-func.func @unsupported_non_contiguous_dim_write(%value : vector<2x2xf32>,
+func.func @regression_non_contiguous_dim_write(%value : vector<2x2xf32>,
----------------
banach-space wrote:

> E.g., there is a CHECK-128B:   memref.collapse_shape in this and other testcases.

But `CHECK-128B` tests a specific configuration of the pass rather than the generic version, right? 

>  Fundamentally, we can not collapse three inner dimensions because the strides is [x, 10, 2, 1] but not [x, 6, 2, 1]

Just to double check - this is referring to `regression_non_contiguous_dim_write`? In which collapsing _does_ happen?

In general, I think that it would be good to identify and document all edge cases through tests (most of the stuff is already here). I would start by renaming this test:

> @regression_non_contiguous_dim_write

"Regression" doesn't really help differentiate this test from other tests. It's a missed opportunity to document what makes this test case unique - i.e. non-zero indices :) (*) So, perhaps:

* `@transfer_write_non_contigous_dims_mismatch_non_zero_idx`

Next, you could move it near `@transfer_write_dims_mismatch_non_contiguous` - both tests seem to check sth very similar, two edge cases for `vector.transfer_write`. I find it very helpful when such edge cases are clustered together.

> > Also, is any testcase testing in_bounds = false and specifically any edge case where the ability to collapse depends on in_bounds ?
>
> There are no negative tests for the case. I can add one to iterate it.

I've been actually discussing this with someone recently :) We don't need it for this PR, but it would be good to add a TODO somewhere (I can look into this).

(*) In general, I'm not a fan of regression tests, but appreciate that others find the valuable.

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