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

[hvdijk]

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.

[hvdijk]

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

[craig.topper]

llvm/test/CodeGen/X86/pic.ll

103

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

[hvdijk]

llvm/test/CodeGen/X86/pic.ll

103

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.

[craig.topper]

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]

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.

[RKSimon]

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

[hvdijk]

In D91924#2418221, @RKSimon wrote:

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]

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.

[hvdijk]

Changed auto to Register

llvm/lib/Target/X86/X86FastISel.cpp

if (const GlobalValue *GV = dyn_cast<GlobalValue>(V)) {
     }
 
     return true;
   }
 
   // If all else fails, try to materialize the value in a register.
   if (!AM.GV || !Subtarget->isPICStyleRIPRel()) {
     if (AM.Base.Reg == 0) {
       AM.Base.Reg = getRegForValue(V);
       return AM.Base.Reg != 0;
     }
     if (AM.IndexReg == 0) {
       assert(AM.Scale == 1 && "Scale with no index!");
       AM.IndexReg = getRegForValue(V);
       return AM.IndexReg != 0;
     }
   }
 
   return false;
 }
 
 
 /// X86SelectStore - Select and emit code to implement store instructions.
 bool X86FastISel::X86SelectStore(const Instruction *I) {
   // Atomic stores need special handling.
   const StoreInst *S = cast<StoreInst>(I);
if (CalleeAM.GV != nullptr) {
     CalleeOp = CalleeAM.Base.Reg;
   } else
     return false;
 
   // Issue the call.
   MachineInstrBuilder MIB;
   if (CalleeOp) {
     // Register-indirect call.
-    unsigned CallOpc = Is64Bit ? X86::CALL64r : X86::CALL32r;
+    unsigned CallOpc = TLI.getPointerTy(DL) == MVT::i64 ? X86::CALL64r : X86::CALL32r;
     MIB = BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(CallOpc))
       .addReg(CalleeOp);
   } else {
     // Direct call.
     assert(GV && "Not a direct call");
     // See if we need any target-specific flags on the GV operand.
     unsigned char OpFlags = Subtarget->classifyGlobalFunctionReference(GV);
 

llvm/test/CodeGen/X86/pic.ll

 ; CHECK-I686-NEXT:  .L3$pb:
 ; CHECK-I686: popl
 ; CHECK-I686: addl  $_GLOBAL_OFFSET_TABLE_+(.L{{.*}}-.L3$pb), %[[REG3:e..]]
 ; CHECK-I686: calll afoo@PLT
 ; CHECK-I686: movl  pfoo@GOT(%[[REG3]]),
 ; CHECK-I686: calll *
 ; CHECK-X32:  callq afoo@PLT
 ; CHECK-X32:  movl  pfoo@GOTPCREL(%rip),
-; CHECK-X32:  callq *
+; CHECK-X32:  call{{l|q}} *
 }
 
 declare void(...)* @afoo(...)
 
 define void @test4() nounwind {
 entry:
     call void(...) @foo()
     ret void