This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Fix exiting OpPatternRewriteDriver::simplifyLocally after first iteration that didn't change the op.
ClosedPublic

Authored by csigg on Oct 22 2020, 1:00 PM.

Details

Summary

Before this change, we would run maxIterations if the first iteration changed the op.
After this change, we exit the loop as soon as an iteration hasn't changed the op.
Assuming that we have reached a fixed point when an iteration doesn't change the op, this doesn't affect correctness.

Diff Detail

Event Timeline

csigg created this revision.Oct 22 2020, 1:00 PM
csigg requested review of this revision.Oct 22 2020, 1:00 PM
rriddle accepted this revision.Oct 22 2020, 1:04 PM

Can you add a line or two to the description for context on the fix? This just helps reach a fixed point faster and shouldn't affect correctness at all.

Thanks for the fix!

mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
302

Can you keep the initialization here anyways? It's a nice to sanity check to always have variables initialized to a good default.

This revision is now accepted and ready to land.Oct 22 2020, 1:04 PM
csigg updated this revision to Diff 300171.Oct 22 2020, 11:29 PM

Keep 'changed' initialized.

csigg edited the summary of this revision. (Show Details)Oct 22 2020, 11:37 PM
csigg marked an inline comment as done.
sanjoy.google added inline comments.Oct 22 2020, 11:49 PM
mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
302

I've heard the opposite advice -- you should leave variables uninitialized when they're not expected to be used, that way asan will let you know if your assumption is violated.

In this trivial case it doesn't matter though.