[PATCH] D12112: x32. Fixes jmp %reg in x32

JF Bastien via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 18 14:49:43 PDT 2015


jfb added inline comments.

================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:2281
@@ +2280,3 @@
+  case ISD::BRIND: {
+    if (Subtarget->isTarget64BitILP32() && !Subtarget->isTargetNaCl()) {
+      const SDValue &Target = Node->getOperand(1);
----------------
Could you comment on why not NaCl? Maybe restyle to be more readable:
```
case ISD::BRIND: {
  if (Subtarget->isTargetNaCl())
    // Not NaCl because...
    break;
  if (Subtarget->isTarget64BitILP32()) {
    // ...
  }
  break;
```

================
Comment at: test/CodeGen/X86/x32-indirectbr.ll:7
@@ +6,3 @@
+; registers. Therefore, x32 CodeGen needs to zero extend indirectbr's target to
+; 64-bit.
+
----------------
It seems like `test2` relies on switch lowering's specific implementation? Is this tested here because it's generated through a different code path than `indirectbr`? I'd like to make sure that the tests isn't brittle if e.g. fast-isel changes, while still making sure you have proper test coverage.

================
Comment at: test/CodeGen/X86/x32-indirectbr.ll:9
@@ +8,3 @@
+
+define i8 @test1() nounwind ssp {
+entry:
----------------
`CHECK-LABEL`

================
Comment at: test/CodeGen/X86/x32-indirectbr.ll:22
@@ +21,3 @@
+; CHECK: movl {{.*}}, %{{e|r}}[[REG:.[^d]*]]{{d?}}
+; CHECK: jmpq *%r[[REG]]
+
----------------
`CHECK-NEXT` for this line.

================
Comment at: test/CodeGen/X86/x32-indirectbr.ll:24
@@ +23,3 @@
+
+define i32 @test2(i32 %idx) nounwind ssp {
+entry:
----------------
`CHECK-LABEL`

================
Comment at: test/CodeGen/X86/x32-indirectbr.ll:56
@@ +55,2 @@
+; CHECK: movl {{.*}}, %{{e|r}}[[REG:.[^d]*]]{{d?}}
+; CHECK: jmpq *%r[[REG]]
----------------
`CHECK-NEXT`


http://reviews.llvm.org/D12112





More information about the llvm-commits mailing list