This is an archive of the discontinued LLVM Phabricator instance.

[mlir][VectorToSCF] Bug in TransferRead lowering fixed
ClosedPublic

Authored by limo1996 on Aug 19 2020, 12:06 AM.

Details

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.

Diff Detail

Event Timeline

limo1996 created this revision.Aug 19 2020, 12:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 12:06 AM
limo1996 requested review of this revision.Aug 19 2020, 12:06 AM

Can we have a test for this?

nicolasvasilache requested changes to this revision.Aug 19 2020, 1:35 AM

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!

This revision now requires changes to proceed.Aug 19 2020, 1:35 AM
limo1996 updated this revision to Diff 286527.Aug 19 2020, 3:47 AM

added test

Can we have a test for this?

Yes! I added one.

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 resigned from this revision.Aug 19 2020, 5:13 AM

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 updated this revision to Diff 286548.Aug 19 2020, 6:27 AM

added test for transfer_write

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 accepted this revision.Aug 19 2020, 6:47 AM

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.

This revision is now accepted and ready to land.Aug 19 2020, 6:47 AM
limo1996 updated this revision to Diff 286565.Aug 19 2020, 8:14 AM

CHECK-NEXT -> CHECK

limo1996 marked an inline comment as done.Aug 19 2020, 8:33 AM