Page MenuHomePhabricator

[MLIR] Handle in-place folding properly in greedy pattern rewrite driver

Authored by bondhugula on Apr 4 2020, 10:09 PM.



OperatioFolder::tryToFold performs both true folding and in a few
instances in-place updates through op rewrites. In the latter case, we
should still be applying the supplied pattern rewrites in the same
iteration; however this wasn't the case since tryToFold returned
success() for both true folding and in-place updates, and the patterns
for the in-place updated ops were being applied only in the next
iteration of the driver's outer loop. This fix would make it converge

Depends on D77478.

Diff Detail

Event Timeline

bondhugula created this revision.Apr 4 2020, 10:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2020, 10:09 PM
bondhugula updated this revision to Diff 255153.Apr 5 2020, 6:49 AM

Rebase and reorder stack.

rriddle added inline comments.Apr 9 2020, 11:02 AM

We could just set this inside of preReplaceAction instead of changing the folder API for now?

bondhugula added inline comments.Apr 10 2020, 12:23 AM

Two quick questions

  1. preReplaceAction is also not called when the folding fails. Did you mean to set 'foldedAndErased = true' in preReplaceAction? Leaving inPlaceUpdate to true when preReplaceAction is not called would be incorrect.
  1. Also, wouldn't a tryToFold client want to know if the op was erased? I had a use case for that but that'll anyway not be needed once the child revision which adds 'op local fold and apply patterns' is available. But otherwise there would be no way to know if the fold erased the op.
rriddle accepted this revision.Apr 10 2020, 11:55 PM
rriddle added inline comments.

Can we just have this as a precondition? That avoids uninitialized variables being passed in.

This revision is now accepted and ready to land.Apr 10 2020, 11:55 PM
bondhugula added inline comments.Apr 11 2020, 12:58 AM

Sorry, I didn't get you. Did you mean making inPlaceUpdate a compulsory parameter and asserting inPlaceUpdate (in which case we can instead just pass by reference) or adding assert(!*inPlaceUpdate && 'should be initialized to false');? Thanks.

rriddle added inline comments.Apr 11 2020, 1:47 AM

I meant the second one.

bondhugula marked 6 inline comments as done.Apr 11 2020, 7:15 AM
bondhugula added inline comments.

Hmm... that would be weird to have a part of its logic outside given inPlaceUpdate is an output parameter (and the assertion would still crash if they just sent in an uninitialized pointer).

bondhugula marked an inline comment as done.Apr 11 2020, 7:16 AM
This revision was automatically updated to reflect the committed changes.