This is an archive of the discontinued LLVM Phabricator instance.

X86FixupBWInsts: Forward liveness analysis is no longer necessary.
ClosedPublic

Authored by MatzeB on Jul 6 2016, 8:02 PM.

Details

Summary

With r274952 and rXXXX in place there are no cases left where a forward
liveness analysis yields different results to a backward one. So we can
remove the forward stepping logic.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 63017.Jul 6 2016, 8:02 PM
MatzeB retitled this revision from to Revert "[X86]: Improve Liveness checking for X86FixupBWInsts.cpp".
MatzeB updated this object.
MatzeB added reviewers: kbsmith1, ab.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.
kbsmith1 edited edge metadata.Jul 6 2016, 9:38 PM

What have you done to prove to yourself that this code is no longer necessary?
If unnecessary, then both before and after the change, the code for 401.bzip2 from spec20006 ought to
be identical.

One way of showing that this code is no longer necessary would be to use the new code and simply
add an assertion after line 389 that NewMI was always nullptr. Once that code had been in for a while,
you could make the extra forward pass be a debug-only code in an attempt to catch other regressions
that might occur in live-reg tracking.

Also, I kind of liked the better abstraction of tryReplaceInstr. I thought that was an improvement, and is another
reason I don't think just a simple revert is the ideal fix.

MatzeB added a comment.Jul 7 2016, 3:50 PM

What have you done to prove to yourself that this code is no longer necessary?
If unnecessary, then both before and after the change, the code for 401.bzip2 from spec20006 ought to
be identical.

I can certainly explain to you what caused these ridiculous live-in lists. Basically looking at a set of register units and then adding all registers with that unit leads to al, ax, eax, rax getting added even if just al is live (additonally the code would unnecessarily add all the pristine registers). So I am pretty sure these wrong live-in lists causes the way too conversative lifeness when walking the block from the end.

One way of showing that this code is no longer necessary would be to use the new code and simply
add an assertion after line 389 that NewMI was always nullptr. Once that code had been in for a while,
you could make the extra forward pass be a debug-only code in an attempt to catch other regressions
that might occur in live-reg tracking.

Also, I kind of liked the better abstraction of tryReplaceInstr. I thought that was an improvement, and is another
reason I don't think just a simple revert is the ideal fix.

Fair enough do a round of testing with the full llvm test-suite including externals (spec95,spec2k,spec2k6) to see that there is really no change. Keeping the tryReplaceInstr() function around is fine, this review is mainly here to get the discussion started :)

MatzeB added a comment.Jul 7 2016, 3:51 PM

What have you done to prove to yourself that this code is no longer necessary?
If unnecessary, then both before and after the change, the code for 401.bzip2 from spec20006 ought to
be identical.

I can certainly explain to you what caused these ridiculous live-in lists. Basically looking at a set of register units and then adding all registers with that unit leads to al, ax, eax, rax getting added even if just al is live (additonally the code would unnecessarily add all the pristine registers). So I am pretty sure these wrong live-in lists causes the way too conversative lifeness when walking the block from the end.

One way of showing that this code is no longer necessary would be to use the new code and simply
add an assertion after line 389 that NewMI was always nullptr. Once that code had been in for a while,
you could make the extra forward pass be a debug-only code in an attempt to catch other regressions
that might occur in live-reg tracking.

Also, I kind of liked the better abstraction of tryReplaceInstr. I thought that was an improvement, and is another
reason I don't think just a simple revert is the ideal fix.

Fair enough do a round of testing with the full llvm test-suite including externals (spec95,spec2k,spec2k6) to see that there is really no change. Keeping the tryReplaceInstr() function around is fine, this review is mainly here to get the discussion started :)

Adding the missing words: "Fair enough, I will do a round of testing..."

OK, sounds reasonable.

MatzeB added a comment.Jul 8 2016, 5:53 PM

I added an assert that the forward liveness analysis would not find additional transformation opportunities. As expected most of the problems were fixed with http://reviews.llvm.org/D22027 in place. However it turned out that there existed another subtle case where forward and backward analysis could disagree, I fixed that one in r274952.

MatzeB updated this revision to Diff 63364.Jul 8 2016, 5:55 PM
MatzeB retitled this revision from Revert "[X86]: Improve Liveness checking for X86FixupBWInsts.cpp" to X86FixupBWInsts: Forward liveness analysis is no longer necessary..
MatzeB updated this object.
MatzeB edited edge metadata.

Reworked the patch, title and description.

kbsmith1 accepted this revision.Jul 8 2016, 9:18 PM
kbsmith1 edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jul 8 2016, 9:18 PM
This revision was automatically updated to reflect the committed changes.
llvm/trunk/lib/Target/X86/X86FixupBWInsts.cpp