This is an archive of the discontinued LLVM Phabricator instance.

[x86] Fixing PR28755 by precomputing the address used in CMPXCHG8B
ClosedPublic

Authored by aivchenk on Sep 30 2016, 4:11 AM.

Details

Summary
The bug arises during register allocation on i686 for
CMPXCHG8B instruction when base pointer is needed. CMPXCHG8B
needs 4 implicit registers (EAX, EBX, ECX, EDX) and a memory address,
plus ESI is reserved as the base pointer. With such constraints the only
way register allocator would do its job successfully is when the addressing
mode of the instruction requires only one register. If that is not the case
- we are emitting additional LEA instruction to compute the address

Diff Detail

Event Timeline

aivchenk updated this revision to Diff 73030.Sep 30 2016, 4:11 AM
aivchenk retitled this revision from to [x86] Fixing PR28755 by precomputing the address used in CMPXCHG8B.
aivchenk updated this object.
aivchenk added a reviewer: qcolombet.
aivchenk added a subscriber: srhines.
qcolombet added inline comments.Sep 30 2016, 9:26 AM
lib/Target/X86/X86ISelLowering.cpp
24615

Split the NFC changes in a different patch.

24734

Use an early return to reduce indentation per LLVM coding style.

24754

Shouldn't we check for is32Bit or something?
Indeed, LEA32r would not be valid for 64-bit arch and the helping regalloc shouldn't be needed for 64-bit arch, right?

test/CodeGen/X86/pr28755.ll
1

Couple of comment on the test case:

  • Give a meaningful name to the file name (like cmpxchg something something)
  • Add a comment on what does this test check (in particular the PR# should be help)
  • Add check line to make sure we generate something sensible
  • Add a run line for when the transformation should not happen (with the appropriate checks as well)
aivchenk updated this revision to Diff 73288.Oct 3 2016, 9:10 AM
aivchenk edited edge metadata.

Addressing Quentin's comments:

aivchenk marked 3 inline comments as done.Oct 3 2016, 9:13 AM
aivchenk added inline comments.
lib/Target/X86/X86ISelLowering.cpp
24615
24754

Right. I checked for "TRI->getBaseRegister() == X86::ESI", which is only true for i686, but I agree that it would be better to check directly for 32 bits

Looks pretty good, just one nitpick.

test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll
21 ↗(On Diff #73288)

Why to you need the regex here?

aivchenk marked 4 inline comments as done.Oct 3 2016, 11:38 AM
aivchenk added inline comments.
test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll
21 ↗(On Diff #73288)

You mean in the checks related to the first function? Well, I kinda wanted to express that the register that we are doing "lea" to is the same register we use for a memory location in cmpxchg8b. And since we don't hardcode the name of the register in EmitInstrWithCustomInserter, I did it as a regexp for esi/edi.

Although it makes me wonder whether it is valid or not to define/use regexp with the same name lower in the test

qcolombet added inline comments.Oct 3 2016, 11:55 AM
test/CodeGen/X86/cmpxchg8b_alloca_regalloc_handling.ll
21 ↗(On Diff #73288)

That's fine. My question was regarding the xor or mov. Why do we expect this to change?
We usually pick whatever codegen does, even if both are valid, so that we know when something change. That way we can check it was intended.

aivchenk updated this revision to Diff 73325.Oct 3 2016, 12:50 PM

Ok, got the point. Attached patch represents what currently is generated for those gen's

aivchenk marked an inline comment as done.Oct 3 2016, 12:52 PM
qcolombet accepted this revision.Oct 3 2016, 4:21 PM
qcolombet edited edge metadata.

One last nitpick, don't need to go through another round of review, just commit directly.

Cheers,
Q.

lib/Target/X86/X86InstrBuilder.h
129

Could you add a comment on which operand are what?
I admit I don't remember at first when looking at the code why we need 1 for Operand + 1, until I realize, right this is the scale :).

This revision is now accepted and ready to land.Oct 3 2016, 4:21 PM

thank you a lot for reviewing this! I will ask smb to commit it after I got approve for D24938 - I'm using getAddressFromInstr and it has to work correctly

This revision was automatically updated to reflect the committed changes.