[PATCH] D38333: [X86] Fix using the SJLJ jump table on x86_64

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26571
 
-  unsigned IReg = MRI->createVirtualRegister(&X86::GR32RegClass);
+  unsigned IReg = MRI->createVirtualRegister(&X86::GR32_NOSPRegClass);
   addFrameReference(BuildMI(DispatchBB, DL, TII->get(X86::MOV32rm), IReg), FI,
----------------
Why the restriction on `SP`?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26594
+
+    if (JTE == MachineJumpTableInfo::EK_BlockAddress) {
+      BuildMI(DispContBB, DL, TII->get(X86::JMP64m))
----------------
I think it might be better to handle this as a switch.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26603
+      assert(JTE == MachineJumpTableInfo::EK_LabelDifference32);
+      unsigned Offset = MRI->createVirtualRegister(&X86::GR32RegClass);
+      unsigned Offset64 = MRI->createVirtualRegister(&X86::GR64RegClass);
----------------
`OReg` might be better to identify that this is a register, not the actual offset.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26604
+      unsigned Offset = MRI->createVirtualRegister(&X86::GR32RegClass);
+      unsigned Offset64 = MRI->createVirtualRegister(&X86::GR64RegClass);
+      unsigned TargetAddr = MRI->createVirtualRegister(&X86::GR64RegClass);
----------------
`OReg64` might be better to identify that this is the register, not the offset.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26605
+      unsigned Offset64 = MRI->createVirtualRegister(&X86::GR64RegClass);
+      unsigned TargetAddr = MRI->createVirtualRegister(&X86::GR64RegClass);
+      BuildMI(DispContBB, DL, TII->get(X86::MOV32rm), Offset)
----------------
`TReg` might be better to identify that this the register, not the actual target address.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26617
+          .addReg(Base);
+      BuildMI(DispContBB, DL, TII->get(X86::JMP64r)).addReg(TargetAddr);
+    }
----------------
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.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26622
+        .addReg(0)
+        .addImm(Subtarget.is64Bit() ? 8 : 4)
+        .addReg(IReg)
----------------
Aren't we guaranteed that `Subtarget.is64Bit()` is `false` here due to the condition to which this else clause belongs is `!Subtarget.is64Bit()`.


================
Comment at: test/CodeGen/X86/sjlj-eh.ll:121
+; CHECK-X64: leaq .[[TABLE:LJTI[0-9]+_[0-9]+]](%rip), %rcx
+; CHECK-X64: movl (%rcx,%rax,4), %eax
+; CHECK-X64: cltq
----------------
Am I not understanding something here?  This looks more like `*Handlers[UFC.__callsite + 1]` as you are indexing by `%rax + 4`.


================
Comment at: test/CodeGen/X86/sjlj-eh.ll:127
+; CHECK-X64-LINUX: leaq .[[TABLE:LJTI[0-9]+_[0-9]+]](%rip), %rcx
+; CHECK-X64-LINUX: jmpq *(%rcx,%rax,8)
----------------
Can you please check the entire sequence for the dispatch?


https://reviews.llvm.org/D38333