Page MenuHomePhabricator

[DAG] Aggressively cleanup dangling node in CombineZExtLogicopShiftLoad.
ClosedPublic

Authored by niravd on Jan 31 2019, 9:18 AM.

Details

Summary

While dangling nodes will eventually be pruned when they are
considered, leaving them disables combines requiring single-use.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Jan 31 2019, 9:18 AM

Can you describe what's happening in these tests? Should we consider the x86 diffs (inc -> add) regressions?
I'm trying to figure out how I managed to get the same set of diffs in D57516. :)

Hmm , weird. I've been looking at reasons why D57367 is doing so much when I found the case generating dangling node. I _was_ a bit surprised that this causes that regression, but it must be the case that dangling nodes persist to ISel and prevent folding. Presumably your change allowed CombineZExtLogicOpShiftLoad to trigger.

In any case, this is obvious cleanup and rebasing past your revert this is NFCish as expected again. I'll land this presently.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 31 2019, 11:35 AM
This revision was automatically updated to reflect the committed changes.

@niravd was committing this intentional?

I see what happened here.
This patch was based on something after rL352766, so it picked up the test diffs that were mistakenly omitted by the first rev of D57516.
On its own, this patch shows no test diffs, so Nirav proceeded with committing this after rL352769.

D57516 has *no* interaction with this patch AFAICT. That makes a lot more sense than both patches tickling exactly the same set of regression tests.

I have a separate x86 patch in progress to avoid the add->inc seen in the other patch.