This is an archive of the discontinued LLVM Phabricator instance.

[X86] Teach X86FixupBWInsts to promote MOV8rr/MOV16rr to MOV32rr.
ClosedPublic

Authored by ab on May 5 2016, 3:00 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

ab updated this revision to Diff 56352.May 5 2016, 3:00 PM
ab retitled this revision from to [X86] Teach X86FixupBWInsts to promote MOV8rr/MOV16rr to MOV32rr..
ab updated this object.
ab added reviewers: kbsmith1, qcolombet.
ab added subscribers: llvm-commits, mzolotukhin.
kbsmith1 edited edge metadata.May 5 2016, 3:18 PM

The code generally looks good. I don't see any tests that explicitly test this code though:

if (TRI->getSubRegIndex(NewSrcReg, OldSrc.getReg()) !=

    TRI->getSubRegIndex(NewDestReg, OldDest.getReg()))
return nullptr;

Definitely need to be sure that this works, and has tests. Don't want

movb %ah, %cl

to accidentally be turned into

movl %eax, %ecx

lib/Target/X86/X86FixupBWInsts.cpp
98 ↗(On Diff #56352)

I think \brief is no longer needed. I realize the others have it. I was just recently told of this, but haven't removed the others.

249 ↗(On Diff #56352)

I think this would be better as .addReg(NewSrcReg); and then the next line can be deleted.

ab updated this revision to Diff 56434.May 6 2016, 10:35 AM
ab edited edge metadata.
  • verify we don't drop implicit ops
  • forward source kill-flag explicitly
  • add separate test
ab updated this revision to Diff 56436.May 6 2016, 10:39 AM
  • on second thought, don't set the kill-flag: killing the sub doesn't say anything about killing the super, I think.
ab marked 2 inline comments as done.May 6 2016, 10:40 AM
ab added inline comments.
lib/Target/X86/X86FixupBWInsts.cpp
96 ↗(On Diff #56434)

Heh, I didn't even notice it! r268754

247 ↗(On Diff #56436)

I usually copy whole operands when possible, to preserve flags; here the only one that matters is kill, and I think it's incorrect to set it on the destination.

So: addReg() indeed!

kbsmith1 accepted this revision.May 6 2016, 10:47 AM
kbsmith1 edited edge metadata.

LGTM.

This revision is now accepted and ready to land.May 6 2016, 10:47 AM
ab marked 2 inline comments as done.May 6 2016, 10:48 AM

Not much changes in the test-suite (+SPEC ref) but a couple improvements:

Benchmark_Exec_TimeDeltaPreviousCurrentStdDev
SingleSource/Benchmarks/BenchmarkGame/nsieve-bits-1.99%0.82470.80830.0037
MultiSource/Benchmarks/nbench/nbench-1.68%6.19936.09490.0085
LNT/CINT2006_ref/445.gobmk-1.16%524.3664518.30371.2114
ab added a comment.May 6 2016, 10:48 AM

Thanks for the review, Kevin!

This revision was automatically updated to reflect the committed changes.