Page MenuHomePhabricator

[mlir] Fix potential crash in hoistRedundantVectorTransfers
ClosedPublic

Authored by ThomasRaoux on Aug 10 2021, 12:00 PM.

Details

Summary

Operations cannot be moved in the middle of function walk.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Aug 10 2021, 12:00 PM
ThomasRaoux requested review of this revision.Aug 10 2021, 12:00 PM
mravishankar accepted this revision.Sep 12 2021, 12:09 PM

Looks fine to me, but I dont have much context on this. Stamping anyway.

This revision is now accepted and ready to land.Sep 12 2021, 12:09 PM
nicolasvasilache requested changes to this revision.Sep 13 2021, 12:56 PM

Seems like you'd rather want to record the ops during the walk and then process them so that you don't have the issue?

The current change seems intrusive as the reordering may have non-trivial implications?

Putting a blocker for now.

This revision now requires changes to proceed.Sep 13 2021, 12:56 PM

Seems like you'd rather want to record the ops during the walk and then process them so that you don't have the issue?

The current change seems intrusive as the reordering may have non-trivial implications?

Putting a blocker for now.

I can record the ops by breaking down moveLoopInvariantCode so that it just returns the ops to hoist without doing it but I don't fully understand how this is better?

Is your concern that the hoisting of transfer op may expose more loop invariant code which could create more opportunity? If that the case we can just move the code doing the loop invariant code motion within the while loop and this should handle all the cases.

Seems like you'd rather want to record the ops during the walk and then process them so that you don't have the issue?

The current change seems intrusive as the reordering may have non-trivial implications?

Putting a blocker for now.

I can record the ops by breaking down moveLoopInvariantCode so that it just returns the ops to hoist without doing it but I don't fully understand how this is better?

Is your concern that the hoisting of transfer op may expose more loop invariant code which could create more opportunity? If that the case we can just move the code doing the loop invariant code motion within the while loop and this should handle all the cases.

Yes, the latter is my concern, moving the code a few lines below under the while but outside of the walk seems good to me.
I don't have data that this will be better, just an intuition.

Address review comment

Seems like you'd rather want to record the ops during the walk and then process them so that you don't have the issue?

The current change seems intrusive as the reordering may have non-trivial implications?

Putting a blocker for now.

I can record the ops by breaking down moveLoopInvariantCode so that it just returns the ops to hoist without doing it but I don't fully understand how this is better?

Is your concern that the hoisting of transfer op may expose more loop invariant code which could create more opportunity? If that the case we can just move the code doing the loop invariant code motion within the while loop and this should handle all the cases.

Yes, the latter is my concern, moving the code a few lines below under the while but outside of the walk seems good to me.
I don't have data that this will be better, just an intuition.

Sounds good, I moved the code into the while loop.

nicolasvasilache accepted this revision.Sep 17 2021, 1:26 AM
This revision is now accepted and ready to land.Sep 17 2021, 1:26 AM