Page MenuHomePhabricator

Revert "[Canonicalizer] Process regions top-down instead of bottom up & reuse existing constants."
ClosedPublic

Authored by bondhugula on Thu, Mar 25, 4:37 AM.

Details

Summary

Revert "[Canonicalizer] Process regions top-down instead of bottom up & reuse existing constants."

This reverts commit 361b7d125b438cda13fa45f13790767a62252be9 by Chris
Lattner <clattner@nondot.org> dated Fri Mar 19 21:22:15 2021 -0700.

The change to the greedy rewriter driver picking a different order was
made without adequate analysis of the trade-offs and experimentation. A
change like this has far reaching consequences on transformation
pipelines, and a major impact upstream and downstream. For eg., one
can’t be sure that it doesn’t slow down a large number of cases by small
amounts or create other issues. More discussion here:
https://llvm.discourse.group/t/speeding-up-canonicalize/3015/25

Reverting this so that improvements to the traversal order can be made
on a clean slate, in bigger steps, and higher bar.

Diff Detail

Event Timeline

bondhugula created this revision.Thu, Mar 25, 4:37 AM
bondhugula requested review of this revision.Thu, Mar 25, 4:37 AM

Updated commit summary.

bondhugula edited the summary of this revision. (Show Details)Thu, Mar 25, 4:59 AM
bondhugula edited the summary of this revision. (Show Details)
bondhugula edited the summary of this revision. (Show Details)
bondhugula edited the summary of this revision. (Show Details)
bondhugula edited the summary of this revision. (Show Details)
stellaraccident accepted this revision.Thu, Mar 25, 7:24 AM
This revision is now accepted and ready to land.Thu, Mar 25, 7:24 AM

I'm accepting because I think this is the right action per the LLVM developer process. Would prefer an additional ack to land.

stella.stamenova accepted this revision.Thu, Mar 25, 8:44 AM

Sounds like a plan. I'm not looking forward to having to undo and then redo all the necessary changes on our end, but we will have the opportunity to do them better with a little more heads up next time.

For those not following here and in discourse, Chris just proposed discussing this at the ODM meeting in ten minutes.