This is an archive of the discontinued LLVM Phabricator instance.

[x32] Fix FrameIndex check in SelectLEA64_32Addr
ClosedPublic

Authored by pavel.v.chupin on Aug 15 2014, 9:02 AM.

Details

Summary

Fixes http://llvm.org/bugs/show_bug.cgi?id=20016 reproducible on new
lea-5.ll case.
Also use RSP/RBP for x32 lea to save 1 byte used for 0x67 prefix in
ESP/EBP case.

Diff Detail

Repository
rL LLVM

Event Timeline

pavel.v.chupin retitled this revision from to [x32] Fix FrameIndex check in SelectLEA64_32Addr.
pavel.v.chupin updated this object.
pavel.v.chupin edited the test plan for this revision. (Show Details)
pavel.v.chupin added subscribers: zinovy.nis, Unknown Object (MLST).
dschuff added inline comments.Aug 15 2014, 11:24 AM
lib/Target/X86/X86RegisterInfo.cpp
495 ↗(On Diff #12560)

should this just be added to the if-else chain above?

test/CodeGen/X86/lea-5.ll
1 ↗(On Diff #12560)

can this test be simplified at all? it should have some comments about what it's testing for, and what e.g. the control flow is for.

Modified code according review comments. Simplified a bit test case.

dschuff edited edge metadata.Aug 18 2014, 1:54 PM

LGTM with one nit.

test/CodeGen/X86/lea-4.ll
1 ↗(On Diff #12625)

this triple should probably still specify linux as the OS, as the others do.

dschuff accepted this revision.Aug 18 2014, 1:54 PM
dschuff edited edge metadata.
This revision is now accepted and ready to land.Aug 18 2014, 1:54 PM
pavel.v.chupin edited edge metadata.

It's appeared that last version of patch is not correct because both needsStackRealignment and Opc == X86::LEA64_32r can be true at the same time. Making this check separate again and adding test revealing that.
Please review.

dschuff added inline comments.Aug 19 2014, 10:12 AM
test/CodeGen/X86/lea-5.ll
5 ↗(On Diff #12665)

Unfortunately this won't work in environments without sed (none of the other tests use sed either). While it's not great, it's probably better just to duplicate this test function with the other alignment. (presumably then you can also de-regex the -40 in the CHECK line?)

Ok. Actually I found couple of tests with sed (e.g. test/Transforms/GCOVProfiling/linezero.ll) so thought it's fine... not sure how they work though when no sed available. I'll change this test.

thanks; yeah I wasn't sure either. I asked on IRC and the consensus seemed to be to just duplicate if the test is fairly small.

test/CodeGen/X86/lea-5.ll
47 ↗(On Diff #12674)

shouldn't this just be X32: ?
you can check it in the same run, and you aren't calling FileCheck with X32-ALIGN64 anyway.

pavel.v.chupin added inline comments.Aug 19 2014, 1:09 PM
test/CodeGen/X86/lea-5.ll
47 ↗(On Diff #12674)

It's called at line 35, but you are right, it's redundant.

LGTM, thanks.

pavel.v.chupin updated this revision to Diff 12697.

Closed by commit rL216065 (authored by pvchupin).