[X86] Fix using the SJLJ jump table on x86_64

[mstorsjo]

Summary

The previous version didn't work if the jump table base address didn't fit in 32 bit, since it was encoded as an immediate offset.

And in case the jump table is encoded as 32 bit label differences, we need to load and add them to the table base first.

This solves the first half of the issues mentioned in PR34720.

Also fix some of the errors pointed out by -verify-machineinstrs, by using GR32_NOSPRegClass.

[mstorsjo]

Handled the case of MachineJumpTableInfo::EK_LabelDifference32 as well, which is what the jump table ends up as on win64/x86.

[RKSimon]

test/CodeGen/X86/sjlj-eh.ll

3 ↗

(On Diff #116932)

According to PR27481, sjlj-eh.ll fails EXPENSIVE_CHECKS builds - please can you add "-verify-machineinstrs" to all the llc commands?

[mstorsjo]

test/CodeGen/X86/sjlj-eh.ll

3 ↗

(On Diff #116932)

Sure, I'll try to look into that and see if I'm capable of figuring out what to fix to silence those issues, while digging into this.

[mstorsjo]

lib/Target/X86/X86ISelLowering.cpp

26605 ↗

(On Diff #116932)

If building with optimizations, this works as intended. If building without optimizations, this actually uses another, uninitialized register here instead of IReg expanded to 64 bit (as done with TargetOpcode::COPY above). Does anybody happen to have a hint about why that happens?

[mstorsjo]

lib/Target/X86/X86ISelLowering.cpp

26605 ↗

(On Diff #116932)

In particular, this issue seems to happen when the value gets spilled onto the stack. Originally the value (IReg) was loaded in eax, but then was spilled onto the stack, reloaded in ecx, but then IReg64 is used as rdx, which actually doesn't contain the intended value.

So without optimization, this gets compiled into:

        movl    -304(%rbp), %eax
        cmpl    $9, %eax
        movl    %eax, -448(%rbp)        # 4-byte Spill
        jae     .LBB4_26
# BB#25:                                #   in Loop: Header=BB4_24 Depth=1
        leaq    .LJTI4_0(%rip), %rax    
        movl    -448(%rbp), %ecx        # 4-byte Reload - into ecx now
        movl    (%rax,%rdx,4), %ecx   # This uses rdx instead of rcx which contains the value we want
        movslq  %ecx, %rdx
        addq    %rax, %rdx
        movl    %ecx, -448(%rbp)        # 4-byte Spill
        jmpq    *%rdx

[mstorsjo]

Fixed the issues in unoptimized builds; it took some time to figure out that I couldn't use TargetOpcode::COPY to zero-extend a 32 bit register into 64 bit, but had to use TargetOpcode::SUBREG_TO_REG. Also now using more virtual registers for each intermediate value instead of reusing them - this seemed to produce more sensible annotated output from different register allocators (and doesn't matter when using optimizations properly). But if you prefer, I can reduce it down to just using as many virtual registers as before.

[mstorsjo]

Also get rid of some of the errors with -verify-machineinstrs.

These remain though, that are unrelated to this patch:

*** Bad machine code: MBB has allocatable live-in, but isn't entry or landing-pad. ***  
- function:    _Z8functionv
- basic block: BB#1 lpad (0x2dedac0)

[compnerd]

lib/Target/X86/X86ISelLowering.cpp

26571 ↗

(On Diff #117125)

Why the restriction on SP?

26594 ↗

(On Diff #117125)

I think it might be better to handle this as a switch.

26603 ↗

(On Diff #117125)

OReg might be better to identify that this is a register, not the actual offset.

26604 ↗

(On Diff #117125)

OReg64 might be better to identify that this is the register, not the offset.

26605 ↗

(On Diff #117125)

TReg might be better to identify that this the register, not the actual target address.

26617 ↗

(On Diff #117125)

I think that this might be complex enough to warrant a bit of MIR in a comment to allow a quicker read of what you are trying to construct.

26622 ↗

(On Diff #117125)

Aren't we guaranteed that Subtarget.is64Bit() is false here due to the condition to which this else clause belongs is !Subtarget.is64Bit().

test/CodeGen/X86/sjlj-eh.ll

121 ↗

(On Diff #117125)

Am I not understanding something here? This looks more like *Handlers[UFC.__callsite + 1] as you are indexing by %rax + 4.

127 ↗

(On Diff #117125)

Can you please check the entire sequence for the dispatch?

[mstorsjo]

lib/Target/X86/X86ISelLowering.cpp

26571 ↗

(On Diff #117125)

If building with -verify-machineinstrs, the verifier will point out that this register is used as an index in a memory operand like (base+IReg*4), and that register can't be of the GR32 class, it has to be GR32_NOSP.

26617 ↗

(On Diff #117125)

Sure, I'll add comments with what the expected instructions look like

26622 ↗

(On Diff #117125)

Oh, indeed - I fixed a few other similar ones but apparently missed this one.

test/CodeGen/X86/sjlj-eh.ll

121 ↗

(On Diff #117125)

No, this does eax = *(rcx + 4*rax)

127 ↗

(On Diff #117125)

Sure, can do

[mstorsjo]

Changed the if to a switch, added comments about what instructions are supposed to be generated, removed the superfluous conditional in the 32 bit codepath, expanded the test for x86_64 linux, renamed register variables as suggested (and renamed Base into BReg).

[compnerd]

Please adjust the comments.

lib/Target/X86/X86ISelLowering.cpp

26624 ↗

(On Diff #117474)

%rcx is not guaranteed, BReg would be better.

26631 ↗

(On Diff #117474)

%rax and %eax are not guaranteed.

26639 ↗

(On Diff #117474)

%rcx and %rax are not guaranteed, BReg and IReg would be better.

26651 ↗

(On Diff #117474)

Please add a newline before this.

%rcx and %rax are not guaranteed, BReg and IReg would be better.

26658 ↗

(On Diff #117474)

%rax, %eax is not guaranteed, OReg64 and OReg would be better.

26661 ↗

(On Diff #117474)

%rcx, %rax -> TReg, OReg64, BReg.

26665 ↗

(On Diff #117474)

%rax -> TReg.

26673 ↗

(On Diff #117474)

%eax -> IReg.

26571 ↗

(On Diff #117125)

Would be nice to get a comment that indicates that because the register is used as an index into a memory operand, SP is not valid.

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

X86TargetLowering::EmitSjLjDispatchBlock(MachineInstr &MI,
   MF->push_back(DispContBB);
   MF->push_back(TrapBB);
 
   // Insert code into the entry block that creates and registers the function
   // context.
   SetupEntryBlockForSjLj(MI, BB, DispatchBB, FI);
 
   // Create the jump table and associated information
-  MachineJumpTableInfo *JTI =
-      MF->getOrCreateJumpTableInfo(getJumpTableEncoding());
+  unsigned JTE = getJumpTableEncoding();
+  MachineJumpTableInfo *JTI = MF->getOrCreateJumpTableInfo(JTE);
   unsigned MJTI = JTI->createJumpTableIndex(LPadList);
 
   const X86RegisterInfo &RI = TII->getRegisterInfo();
   // Add a register mask with no preserved registers.  This results in all
   // registers being marked as clobbered.
   if (RI.hasBasePointer(*MF)) {
     const bool FPIs64Bit =
         Subtarget.isTarget64BitLP64() || Subtarget.isTargetNaCl64();
     X86MachineFunctionInfo *MFI = MF->getInfo<X86MachineFunctionInfo>();
     MFI->setRestoreBasePointer(MF);
 
     unsigned FP = RI.getFrameRegister(*MF);
     unsigned BP = RI.getBaseRegister();
     unsigned Op = FPIs64Bit ? X86::MOV64rm : X86::MOV32rm;
     addRegOffset(BuildMI(DispatchBB, DL, TII->get(Op), BP), FP, true,
                  MFI->getRestoreBasePointerOffset())
         .addRegMask(RI.getNoPreservedMask());
   } else {
     BuildMI(DispatchBB, DL, TII->get(X86::NOOP))
         .addRegMask(RI.getNoPreservedMask());
   }
 
-  unsigned IReg = MRI->createVirtualRegister(&X86::GR32RegClass);
+  // IReg is used as an index in a memory operand and therefore can't be SP
+  unsigned IReg = MRI->createVirtualRegister(&X86::GR32_NOSPRegClass);
   addFrameReference(BuildMI(DispatchBB, DL, TII->get(X86::MOV32rm), IReg), FI,
                     Subtarget.is64Bit() ? 8 : 4);
   BuildMI(DispatchBB, DL, TII->get(X86::CMP32ri))
       .addReg(IReg)
       .addImm(LPadList.size());
   BuildMI(DispatchBB, DL, TII->get(X86::JAE_1)).addMBB(TrapBB);
 
-  BuildMI(DispContBB, DL,
-          TII->get(Subtarget.is64Bit() ? X86::JMP64m : X86::JMP32m))
+  if (Subtarget.is64Bit()) {
+    unsigned BReg = MRI->createVirtualRegister(&X86::GR64RegClass);
+    unsigned IReg64 = MRI->createVirtualRegister(&X86::GR64_NOSPRegClass);
+
+    // leaq .LJTI0_0(%rip), BReg
+    BuildMI(DispContBB, DL, TII->get(X86::LEA64r), BReg)
+        .addReg(X86::RIP)
+        .addImm(1)
+        .addReg(0)
+        .addJumpTableIndex(MJTI)
+        .addReg(0);
+    // movzx IReg64, IReg
+    BuildMI(DispContBB, DL, TII->get(TargetOpcode::SUBREG_TO_REG), IReg64)
+        .addImm(0)
+        .addReg(IReg)
+        .addImm(X86::sub_32bit);
+
+    switch (JTE) {
+    case MachineJumpTableInfo::EK_BlockAddress:
+      // jmpq *(BReg,IReg64,8)
+      BuildMI(DispContBB, DL, TII->get(X86::JMP64m))
+          .addReg(BReg)
+          .addImm(8)
+          .addReg(IReg64)
+          .addImm(0)
+          .addReg(0);
+      break;
+    case MachineJumpTableInfo::EK_LabelDifference32: {
+      unsigned OReg = MRI->createVirtualRegister(&X86::GR32RegClass);
+      unsigned OReg64 = MRI->createVirtualRegister(&X86::GR64RegClass);
+      unsigned TReg = MRI->createVirtualRegister(&X86::GR64RegClass);
+
+      // movl (BReg,IReg64,4), OReg
+      BuildMI(DispContBB, DL, TII->get(X86::MOV32rm), OReg)
+          .addReg(BReg)
+          .addImm(4)
+          .addReg(IReg64)
+          .addImm(0)
+          .addReg(0);
+      // movsx OReg64, OReg
+      BuildMI(DispContBB, DL, TII->get(X86::MOVSX64rr32), OReg64).addReg(OReg);
+      // addq BReg, OReg64, TReg
+      BuildMI(DispContBB, DL, TII->get(X86::ADD64rr), TReg)
+          .addReg(OReg64)
+          .addReg(BReg);
+      // jmpq *TReg
+      BuildMI(DispContBB, DL, TII->get(X86::JMP64r)).addReg(TReg);
+      break;
+    }
+    default:
+      llvm_unreachable("Unexpected jump table encoding");
+    }
+  } else {
+    // jmpl *.LJTI0_0(,IReg,4)
+    BuildMI(DispContBB, DL, TII->get(X86::JMP32m))
         .addReg(0)
-      .addImm(Subtarget.is64Bit() ? 8 : 4)
+        .addImm(4)
         .addReg(IReg)
         .addJumpTableIndex(MJTI)
         .addReg(0);
+  }
 
   // Add the jump table entries as successors to the MBB.
   SmallPtrSet<MachineBasicBlock *, 8> SeenMBBs;
   for (auto &LP : LPadList)
     if (SeenMBBs.insert(LP).second)
       DispContBB->addSuccessor(LP);
 
   // N.B. the order the invoke BBs are processed in doesn't matter here.

llvm/trunk/test/CodeGen/X86/sjlj-eh.ll

 ; RUN: llc -mtriple i386-windows-gnu -exception-model sjlj -filetype asm -o - %s | FileCheck %s
 ; RUN: llc -mtriple x86_64-windows-gnu -exception-model sjlj -filetype asm -o - %s | FileCheck %s -check-prefix CHECK-X64
+; RUN: llc -mtriple x86_64-linux -exception-model sjlj -filetype asm -o - %s | FileCheck %s -check-prefix CHECK-X64-LINUX
 
 declare void @_Z20function_that_throwsv()
 declare i32 @__gxx_personality_sj0(...)
 declare i8* @__cxa_begin_catch(i8*)
 declare void @__cxa_end_catch()
 
 define void @_Z8functionv() personality i8* bitcast (i32 (...)* @__gxx_personality_sj0 to i8*) {
 entry:

 ; CHECK-X64: [[RESUME]]:
 ;     assert(UFC.__callsite < 1);
 ; CHECK-X64: movl -304(%rbp), %eax
 ; CHECK-X64: cmpl $1, %eax
 ; CHECK-X64: jb .[[CONT:LBB[0-9]+_[0-9]+]]
 ; CHECK-X64: ud2
 ; CHECK-X64: [[CONT]]:
 ;     *Handlers[UFC.__callsite]
-; CHECK-X64: jmpq *.LJTI
+; CHECK-X64: leaq .[[TABLE:LJTI[0-9]+_[0-9]+]](%rip), %rcx
+; CHECK-X64: movl (%rcx,%rax,4), %eax
+; CHECK-X64: cltq
+; CHECK-X64: addq %rcx, %rax
+; CHECK-X64: jmpq *%rax
+
+; CHECK-X64-LINUX: .[[RESUME:LBB[0-9]+_[0-9]+]]:
+;     assert(UFC.__callsite < 1);
+; CHECK-X64-LINUX: movl -120(%rbp), %eax
+; CHECK-X64-LINUX: cmpl $1, %eax
+; CHECK-X64-LINUX: jb .[[CONT:LBB[0-9]+_[0-9]+]]
+; CHECK-X64-LINUX: ud2
+; CHECK-X64-LINUX: [[CONT]]:
+;     *Handlers[UFC.__callsite]
+; CHECK-X64-LINUX: leaq .[[TABLE:LJTI[0-9]+_[0-9]+]](%rip), %rcx
+; CHECK-X64-LINUX: jmpq *(%rcx,%rax,8)