This is an archive of the discontinued LLVM Phabricator instance.

x32. Fixes jmp %reg in x32
ClosedPublic

Authored by jpp on Aug 18 2015, 1:06 PM.

Details

Summary

x32 has 32-bit pointers; x86-64 can't jmp %r32. This patch addresses this issue by explicitly zero-extending brind's target to 64-bits.

Diff Detail

Repository
rL LLVM

Event Timeline

jpp updated this revision to Diff 32441.Aug 18 2015, 1:06 PM
jpp retitled this revision from to x32. Fixes jmp %reg in x32.
jpp updated this object.
jpp added a subscriber: llvm-commits.
jpp updated this object.Aug 18 2015, 1:10 PM
dschuff added inline comments.Aug 18 2015, 1:59 PM
test/CodeGen/X86/x32-indirectbr.ll
21 ↗(On Diff #32441)

%{{e|r}}
if you are checking for a movl, then %rXX shouldn't be allowed, should it?

jpp added inline comments.Aug 18 2015, 2:08 PM
test/CodeGen/X86/x32-indirectbr.ll
21 ↗(On Diff #32441)

They are, if you are using the new registers( r8d-15d). The regexp could potentially be improved.

Adding Pavel, if Intel is still interested in moving x32 forward :-)

jfb added inline comments.Aug 18 2015, 2:49 PM
lib/Target/X86/X86ISelDAGToDAG.cpp
2281 ↗(On Diff #32441)

Could you comment on why not NaCl? Maybe restyle to be more readable:

case ISD::BRIND: {
  if (Subtarget->isTargetNaCl())
    // Not NaCl because...
    break;
  if (Subtarget->isTarget64BitILP32()) {
    // ...
  }
  break;
test/CodeGen/X86/x32-indirectbr.ll
7 ↗(On Diff #32441)

It seems like test2 relies on switch lowering's specific implementation? Is this tested here because it's generated through a different code path than indirectbr? I'd like to make sure that the tests isn't brittle if e.g. fast-isel changes, while still making sure you have proper test coverage.

9 ↗(On Diff #32441)

CHECK-LABEL

22 ↗(On Diff #32441)

CHECK-NEXT for this line.

24 ↗(On Diff #32441)

CHECK-LABEL

56 ↗(On Diff #32441)

CHECK-NEXT

dschuff added inline comments.Aug 18 2015, 2:55 PM
test/CodeGen/X86/x32-indirectbr.ll
21 ↗(On Diff #32441)

Oh ok, I misunderstood. and the regex is explicitly trying to throw away the d for REG. It makes sense, but probably worth a comment saying that the test is checking for a 32-bit mov followed by a jump through the 64-bit version of the same register.

jpp updated this revision to Diff 32465.Aug 18 2015, 3:36 PM
jpp marked 7 inline comments as done.
jfb accepted this revision.Aug 18 2015, 3:42 PM
jfb added a reviewer: jfb.

lgtm

This revision is now accepted and ready to land.Aug 18 2015, 3:42 PM
This revision was automatically updated to reflect the committed changes.