[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

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

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

26652

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

26652

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

Why the restriction on SP?

26594

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

26603

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

26604

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

26605

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

26617

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

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

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

127

Can you please check the entire sequence for the dispatch?

[mstorsjo]

lib/Target/X86/X86ISelLowering.cpp

26571

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

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

26622

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

test/CodeGen/X86/sjlj-eh.ll

121

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

127

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

26571

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.

26624

%rcx is not guaranteed, BReg would be better.

26631

%rax and %eax are not guaranteed.

26639

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

26651

Please add a newline before this.

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

26658

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

26661

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

26665

%rax -> TReg.

26673

%eax -> IReg.

lib/Target/X86/X86ISelLowering.cpp

   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);
+  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))
-      .addReg(0)
-      .addImm(Subtarget.is64Bit() ? 8 : 4)
-      .addReg(IReg)
-      .addJumpTableIndex(MJTI)
-      .addReg(0);
+  if (Subtarget.is64Bit()) {
+    unsigned Base = MRI->createVirtualRegister(&X86::GR64RegClass);
+    unsigned IReg64 = MRI->createVirtualRegister(&X86::GR64_NOSPRegClass);
+
+    BuildMI(DispContBB, DL, TII->get(X86::LEA64r), Base)
+        .addReg(X86::RIP)
+        .addImm(1)
+        .addReg(0)
+        .addJumpTableIndex(MJTI)
+        .addReg(0);
+    BuildMI(DispContBB, DL, TII->get(TargetOpcode::SUBREG_TO_REG), IReg64)
+        .addImm(0)
+        .addReg(IReg)
+        .addImm(X86::sub_32bit);
+
+    if (JTE == MachineJumpTableInfo::EK_BlockAddress) {
+      BuildMI(DispContBB, DL, TII->get(X86::JMP64m))
+          .addReg(Base)
+          .addImm(8)
+          .addReg(IReg64)
+          .addImm(0)
+          .addReg(0);
+    } else {
+      assert(JTE == MachineJumpTableInfo::EK_LabelDifference32);
+      unsigned Offset = MRI->createVirtualRegister(&X86::GR32RegClass);
+      unsigned Offset64 = MRI->createVirtualRegister(&X86::GR64RegClass);
+      unsigned TargetAddr = MRI->createVirtualRegister(&X86::GR64RegClass);
+      BuildMI(DispContBB, DL, TII->get(X86::MOV32rm), Offset)
+          .addReg(Base)
+          .addImm(4)
+          .addReg(IReg64)
+          .addImm(0)
+          .addReg(0);
+      BuildMI(DispContBB, DL, TII->get(X86::MOVSX64rr32), Offset64)
+          .addReg(Offset);
+      BuildMI(DispContBB, DL, TII->get(X86::ADD64rr), TargetAddr)
+          .addReg(Offset64)
+          .addReg(Base);
+      BuildMI(DispContBB, DL, TII->get(X86::JMP64r)).addReg(TargetAddr);
+    }
+  } else {
+    BuildMI(DispContBB, DL, TII->get(X86::JMP32m))
+        .addReg(0)
+        .addImm(Subtarget.is64Bit() ? 8 : 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.
   SmallVector<MachineBasicBlock *, 64> MBBLPads;
   const MCPhysReg *SavedRegs = MF->getRegInfo().getCalleeSavedRegs();
   for (MachineBasicBlock *MBB : InvokeBBs) {
     // Remove the landing pad successor from the invoke block and replace it
     // with the new dispatch block.
     // Keep a copy of Successors since it's modified inside the loop.
     SmallVector<MachineBasicBlock *, 8> Successors(MBB->succ_rbegin(),
                                                    MBB->succ_rend());
     // FIXME: Avoid quadratic complexity.
     for (auto MBBS : Successors) {
       if (MBBS->isEHPad()) {
         MBB->removeSuccessor(MBBS);
         MBBLPads.push_back(MBBS);
       }
     }
 
     MBB->addSuccessor(DispatchBB);
 
     // Find the invoke call and mark all of the callee-saved registers as
     // 'implicit defined' so that they're spilled.  This prevents code from
     // moving instructions to before the EH block, where they will never be
     // executed.
     for (auto &II : reverse(*MBB)) {
       if (!II.isCall())
         continue;
 
       DenseMap<unsigned, bool> DefRegs;
       for (auto &MOp : II.operands())
         if (MOp.isReg())
           DefRegs[MOp.getReg()] = true;
 
       MachineInstrBuilder MIB(*MF, &II);
       for (unsigned RI = 0; SavedRegs[RI]; ++RI) {
         unsigned Reg = SavedRegs[RI];
         if (!DefRegs[Reg])
           MIB.addReg(Reg, RegState::ImplicitDefine | RegState::Dead);
       }
 
       break;
     }
   }
 
   // Mark all former landing pads as non-landing pads.  The dispatch is the only
   // landing pad now.
   for (auto &LP : MBBLPads)
     LP->setIsEHPad(false);
 

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: leaq .[[TABLE:LJTI[0-9]+_[0-9]+]](%rip), %rcx
+; CHECK-X64-LINUX: jmpq *(%rcx,%rax,8)