Page MenuHomePhabricator

[X86] Have indirect calls take 64-bit operands in 64-bit modes
ClosedPublic

Authored by hvdijk on Sat, Nov 21, 1:58 PM.

Details

Summary

The build bots caught two additional pre-existing problems exposed by the test change part of my change https://reviews.llvm.org/D91339, when expensive checks are enabled. This fixes one of them.

X86 has CALL64r and CALL32r opcodes, where CALL64r takes a 64-bit register, and CALL32r takes a 32-bit register. CALL64r can only be used in 64-bit mode, CALL32r can only be used in 32-bit mode. LLVM would assume that after picking the appropriate CALLr opcode, a pointer-sized register would be a valid operand, but in x32 mode, a 64-bit mode, pointers are 32 bits. In this mode, it is invalid to directly pass a pointer to CALL64r, it needs to be extended to 64 bits first.

Diff Detail

Event Timeline

hvdijk created this revision.Sat, Nov 21, 1:58 PM
hvdijk requested review of this revision.Sat, Nov 21, 1:58 PM
hvdijk updated this revision to Diff 306876.Sat, Nov 21, 2:02 PM

Forgot to run clang-format-diff before uploading, updated now.

craig.topper added inline comments.Sat, Nov 21, 2:32 PM
llvm/test/CodeGen/X86/pic.ll
103 ↗(On Diff #306876)

Why a regular expression? Shouldn't it be fixed for x32?

hvdijk added inline comments.Sat, Nov 21, 2:55 PM
llvm/test/CodeGen/X86/pic.ll
103 ↗(On Diff #306876)

It's a good question. I wanted to say that either would be okay since the high bits of all addresses are known to be zero, but actually that is not at all the case at all, it must be callq, and this patch is wrong. The instruction would be okay if it were valid, but it's not: the result of CALL32r passes LLVM's own consistency tests, but is subsequently rejected by the assembler, something not covered by the tests. I will redo this patch to use the other approach, the zero extending. Apologies for the noise and for the breakage.

hvdijk updated this revision to Diff 306887.Sat, Nov 21, 5:02 PM
hvdijk retitled this revision from [X86] Have indirect calls take pointer-sized operands to [X86] Have indirect calls take 64-bit operands in 64-bit modes.
hvdijk edited the summary of this revision. (Show Details)

Is there a test case?

llvm/lib/Target/X86/X86FastISel.cpp
1092

SUBREG_TO_REG is sort of an assertion that the upper 32 bits of the register are 0. This would require the producing instruction to definitely be a 32-bit operation and not some 64-bit operation that was just truncated to 32-bits. Is that really guaranteed here?

hvdijk updated this revision to Diff 306896.Sun, Nov 22, 1:58 AM

Add MOV32rr to guarantee high bits will be zero.

llvm/lib/Target/X86/X86FastISel.cpp
1092

It took a bit of effort to find a test case to verify, but you're right, it's not. Even if getRegForValue will create a new instruction, and all hardware instructions that set a 32-bit register will implicitly zero the high 32 bits, getRegForValue need not create a hardware instruction, it may create a pseudo-instructions such as a subreg copy.

We already have fast-isel-call tests - could we add gnux32 test coverage there?

RKSimon added a reviewer: glaubitz.

We already have fast-isel-call tests - could we add gnux32 test coverage there?

I took a look, and none of them would be affected by this change (none of them use PIC or indirect calls). It does seem like it might be a useful separate improvement, but it's a bit tricky: fast-isel-call.ll tests x86-specific calling conventions so probably wouldn't work, fast-isel-call-bool.ll and fast-isel-call-cleanup.ll look like they test for specific bugs that may or may not be/have been a problem for x32.

(For completeness, I mentioned it already but not in the description of this diff: this change is covered by a test, this fixes a problem exposed by pic.ll.)

RKSimon accepted this revision.Fri, Nov 27, 3:00 AM

Thanks for the clarification - LGTM with a couple of minors

llvm/lib/Target/X86/X86FastISel.cpp
1086

(style) Avoid auto - just use Register.

1090

(style) Avoid auto - just use Register.

1095

(style) Avoid auto - just use Register.

This revision is now accepted and ready to land.Fri, Nov 27, 3:00 AM
hvdijk updated this revision to Diff 308093.Fri, Nov 27, 11:12 AM

Changed auto to Register

hvdijk marked 4 inline comments as done.Fri, Nov 27, 11:13 AM