[mlir][VectorToSCF] Bug in TransferRead lowering fixed

[limo1996]

Summary

If Memref has rank > 1 this pass emits N-1 loops around
TransferRead op and transforms the op itself to 1D read. Since vectors
must have static shape while memrefs don't the pass emits if condition
to prevent out of bounds accesses in case some memref dimension is smaller
than the corresponding dimension of targeted vector. This logic is fine
but authors forgot to apply permutation_map on loops upper bounds and
thus if condition compares induction variable to incorrect loop upper bound
(dimension of the memref) in case permutation_map is not identity map.
This commit aims to fix that.

[limo1996]

update

[ftynse]

Can we have a test for this?

[nicolasvasilache]

There are a few different places where patterns apply where we make the test that the permutation map is identity and that the rest is not yet supported.
Please update such places too and add tests to extend support more generally.

Thanks for extending this functionality!

[limo1996]

added test

[limo1996]

In D86197#2225657, @ftynse wrote:

Can we have a test for this?

Yes! I added one.

[limo1996]

In D86197#2225689, @nicolasvasilache wrote:

There are a few different places where patterns apply where we make the test that the permutation map is identity and that the rest is not yet supported.
Please update such places too and add tests to extend support more generally.

Thanks for extending this functionality!

I did not quite get the comment probably because it is too generic. I don't see any checks for isIdentity in the vector-to-scf pass only for isMinorIdentity which happens once for TransferRead and once for TransferWrite. I am technically also not extending the functionality but rather fixing what should be supported. I can fix it in other places as well but can you be please more concreate if those places are in this lowering pass or also in some other one? Or you meant fixing it for TransferWrite as well? Thanks.

[nicolasvasilache]

I was under the impression that you were implementing the more general case but indeed not.
Then please just make sure the writes behave properly too, thanks!

Resigning as a reviewer to unblock as I am OOO until early Sept.

[limo1996]

added test for transfer_write

[limo1996]

In D86197#2225972, @nicolasvasilache wrote:

I was under the impression that you were implementing the more general case but indeed not.
Then please just make sure the writes behave properly too, thanks!

Resigning as a reviewer to unblock as I am OOO until early Sept.

Ok. I found out that the change I made works fine also for vector.transfer_write and therefore I just added a test case for that.

[ftynse]

Thanks for the fix!

mlir/test/Conversion/VectorToSCF/vector-to-loops.mlir

415

I don't think we care that this is on the next line, so I'd drop -NEXT from checks here and below.

[limo1996]

CHECK-NEXT -> CHECK

[limo1996]

Landed as 8dace28f92056a04e02d913684d3f448ee1f295a