Page MenuHomePhabricator

[X86]: Improve Liveness checking for X86FixupBWInsts.cpp
ClosedPublic

Authored by kbsmith1 on Jun 7 2016, 10:35 AM.

Details

Summary

This improves the liveness checking for X86FixupBWInsts.cpp. Sometimes computing liveness
going backwards is overly conservative because the live-outs of the block can contain super-regs
that are not really live. This change finds candidates for BW Fixup where the tranform failed, and
when that happens, runs the liveness going forward to see if that gives a better answer.

Diff Detail

Event Timeline

kbsmith1 updated this revision to Diff 59907.Jun 7 2016, 10:35 AM
kbsmith1 retitled this revision from to [X86]: Improve Liveness checking for X86FixupBWInsts.cpp.
kbsmith1 updated this object.
kbsmith1 added reviewers: echristo, ab, DavidKreitzer.
ab requested changes to this revision.Jun 7 2016, 12:13 PM
ab edited edge metadata.

Can you add a testcase?

I'm hoping this is better fixed elsewhere, and we can avoid relying on kill-flags.

lib/Target/X86/X86FixupBWInsts.cpp
102–103

Have you considered returning MI itself to signify 'WasCandidate = true' ?

374

auto ?

This revision now requires changes to proceed.Jun 7 2016, 12:13 PM

I'd love to add a test case. Of the spec2000 in tests, 175, 176, 252, 254, 256, and 300 all hit instances where
this new code finds new opportunities when going forward, that are missed when going backwards.

Unfortunately, so far, in looking at the cases, they are all quite large functions, and not at all amenable to
really being unit test cases.

And whether that would continue to check this code would depend on the vagaries of register allocation.

And, if you have better suggestions for how to fix this elsewhere, that would be great. The only way I really know
to solve this problem properly is for this transform to run its own, very specific data-flow.

lib/Target/X86/X86FixupBWInsts.cpp
102–103

That seems more obscure than simply having this also return the bool WasCandidate. I like the code this way because it is simple.

374

Sure, done.

kbsmith1 updated this revision to Diff 59964.Jun 7 2016, 3:42 PM
kbsmith1 edited edge metadata.

Added minimalist (as possible) test, and changed to use auto as suggested.

echristo edited edge metadata.Jun 8 2016, 1:28 PM

One inline comment.

-eric

test/CodeGen/X86/fixup-bw-inst-fwlive.ll
12–13

Can you add some more documentation on what the checks are rather than "complex" for the next person that has to do something with this testcase? :)

kbsmith1 updated this revision to Diff 60098.Jun 8 2016, 1:37 PM
kbsmith1 edited edge metadata.

Updated the comment in the test.

RKSimon added a subscriber: RKSimon.Jun 8 2016, 2:36 PM
ab added a comment.Jun 10 2016, 8:31 AM

This seems perfect for MIR: you could have a tiny test with two blocks, a MOV8rr and a spurious live-out.
There's one for FixupBW in test/CodeGen/X86/fixup-bw-copy.mir, but test/CodeGen/X86/eflags-copy-expansion.mir might be more a better start.

Other than that, I have no objection to the patch, but I'm worried we're papering over liveness deficiencies. Realistically though, we can't fix everything, so this is probably the best we can do right now.

As to this specific test: looks like BranchFolding, when merging tails, indiscriminately marks every register defined but not killed (it might really be live, but not necessarily) before the tail as live-in the new tail block. It queries the RegisterScavenger, which walks the original block forward, using kill flags; maybe it should walk the tail block backward instead? I might have missed something though; I'll ask Matthias or Quentin. I also have no idea if it fixes every problems you've seen.

lib/Target/X86/X86FixupBWInsts.cpp
337–339

Drop the typedef?

kbsmith1 updated this revision to Diff 60370.Jun 10 2016, 10:08 AM

Addressed comments from Ahmed. Got rid of typedef, and created MIR based test.

Ping. I believe I have addressed all comments.

ab accepted this revision.Jun 15 2016, 8:30 AM
ab edited edge metadata.

I filed https://llvm.org/PR28142 for the BranchFolding/RegScavenger issue. Can you add a FIXME somewhere linking to that?

With that, LGTM.

This revision is now accepted and ready to land.Jun 15 2016, 8:30 AM
This revision was automatically updated to reflect the committed changes.