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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd accepted this revision.
compnerd added a comment.
This revision is now accepted and ready to land.

Please adjust the 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,
----------------
mstorsjo wrote:
> 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.
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.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26624
+
+    // leaq .LJTI0_0(%rip), %rcx
+    BuildMI(DispContBB, DL, TII->get(X86::LEA64r), BReg)
----------------
`%rcx` is not guaranteed, `BReg` would be better.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26631
+        .addReg(0);
+    // movzx %rax, %eax
+    BuildMI(DispContBB, DL, TII->get(TargetOpcode::SUBREG_TO_REG), IReg64)
----------------
`%rax` and `%eax` are not guaranteed.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26639
+    case MachineJumpTableInfo::EK_BlockAddress:
+      // jmpq *(%rcx,%rax,8)
+      BuildMI(DispContBB, DL, TII->get(X86::JMP64m))
----------------
`%rcx` and `%rax` are not guaranteed, `BReg` and `IReg` would be better.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26651
+      unsigned TReg = MRI->createVirtualRegister(&X86::GR64RegClass);
+      // movl (%rcx,%rax,4), %eax
+      BuildMI(DispContBB, DL, TII->get(X86::MOV32rm), OReg)
----------------
Please add a newline before this.

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


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26658
+          .addReg(0);
+      // movsx %rax, %eax
+      BuildMI(DispContBB, DL, TII->get(X86::MOVSX64rr32), OReg64)
----------------
`%rax`, `%eax` is not guaranteed, `OReg64` and `OReg` would be better.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26661
+          .addReg(OReg);
+      // addq %rcx, %rax
+      BuildMI(DispContBB, DL, TII->get(X86::ADD64rr), TReg)
----------------
`%rcx`, `%rax` -> `TReg`, `OReg64`, `BReg`.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26665
+          .addReg(BReg);
+      // jmpq *%rax
+      BuildMI(DispContBB, DL, TII->get(X86::JMP64r)).addReg(TReg);
----------------
`%rax` -> `TReg`.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26673
+  } else {
+    // jmpl *.LJTI0_0(,%eax,4)
+    BuildMI(DispContBB, DL, TII->get(X86::JMP32m))
----------------
`%eax` -> `IReg`.


https://reviews.llvm.org/D38333