Page MenuHomePhabricator

[InstCombine] When rewriting operands in SimplifyAssociativeOrCommutative, add the original nodes back to the worklist so they'll get eagerly DCEd
Changes PlannedPublic

Authored by craig.topper on Aug 20 2018, 12:29 PM.

Details

Reviewers
spatel
Summary

If the original operands become dead they'll stick around until the next iteration of the InstCombine worklist. The worklist builder for that iteration will DCE the last dead node. But if that node was the only thing keeping an earlier node alive, the earlier node will have already been added to the worklist. So earlier nodes won't be DCEd until they are popped from the worklist. Any DCE from the worklist will be counted as a modification that will trigger another iteration of InstCombine even if nothing else was changed.

This patch explicitly adds the nodes back to the worklist so they will be DCEd in the worklist iteration they become dead. The worklist DCE code should take care of adding dead operands to the worklist so the process will remove all dead nodes. This will prevent the worklist builder for the next iteration from having any dead nodes from this. So we can stop another InstCombine iteration from being triggered.

There other places that use setOperand that are probably subject to this same problem. I know we've tried to rewrite operands to save allocations.

Diff Detail

Event Timeline

craig.topper created this revision.Aug 20 2018, 12:29 PM
spatel added a comment.EditedAug 21 2018, 6:04 AM

Is it possible to show the difference using debug output? If not, I think we should at least have a (small?) example here in the review, so there's some record of the problem for future reference.

craig.topper planned changes to this revision.Oct 1 2018, 10:21 AM

Need to find the test case I observed this on originally.