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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 4 inline comments as done.
mstorsjo 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,
----------------
compnerd wrote:
> Why the restriction on `SP`?
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.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26617
+          .addReg(Base);
+      BuildMI(DispContBB, DL, TII->get(X86::JMP64r)).addReg(TargetAddr);
+    }
----------------
compnerd wrote:
> 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.
Sure, I'll add comments with what the expected instructions look like


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26622
+        .addReg(0)
+        .addImm(Subtarget.is64Bit() ? 8 : 4)
+        .addReg(IReg)
----------------
compnerd wrote:
> Aren't we guaranteed that `Subtarget.is64Bit()` is `false` here due to the condition to which this else clause belongs is `!Subtarget.is64Bit()`.
Oh, indeed - I fixed a few other similar ones but apparently missed this one.


================
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
----------------
compnerd wrote:
> Am I not understanding something here?  This looks more like `*Handlers[UFC.__callsite + 1]` as you are indexing by `%rax + 4`.
No, this does `eax = *(rcx + 4*rax)`


================
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)
----------------
compnerd wrote:
> Can you please check the entire sequence for the dispatch?
Sure, can do


https://reviews.llvm.org/D38333