[PATCH] D134915: [X86] Do not emit JCC to __x86_indirect_thunk

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/test/CodeGen/X86/jcc-indirect-thunk-kernel.ll:12
+; CHECK-NEXT:    bb.2: # %if.then
+; CHECK-NEXT:    jmp __x86_indirect_thunk_r11
+; CHECK-NEXT:    LBB0_1:
----------------
Now consider either adding a comment that "the intent of the test is that we do not generate conditional tail calls to the thunk" or an explicit `CHECK-NOT: jne	__x86_indirect_thunk_r11` line (triple check that would not get removed by update_llc_checks) or both.


================
Comment at: llvm/test/CodeGen/X86/jcc-indirect-thunk-kernel.ll:16-17
+entry:
+  %0 = load void()*, void()** %something, align 8
+  %tobool.not = icmp eq void()* %0, null
+  br i1 %tobool.not, label %if.end, label %if.then
----------------
I'm surprised to see typed pointers, since we've enabled [[ https://llvm.org/docs/OpaquePointers.html | opaque pointers ]]. Was this test case generated from an older release of llvm? I'd have expected

```
%0 = load ptr, ptr %something, align 8
%tobool.not = icmp eq ptr %0, null
```
no `void()*` or `void()**`. Please change this, and triple check if you're using an older version of llvm for development, or have something wrong in your cmakecache. We'd like to eliminate non-opaque ptrs from the codebase, IIUC.


================
Comment at: llvm/test/CodeGen/X86/jcc-indirect-thunk-kernel.ll:1
+; RUN: llc < %s -O2 --code-model=kernel | FileCheck %s
+
----------------
joaomoreira wrote:
> nickdesaulniers wrote:
> > Compile for performance...
> what do you mean exactly?
ah, nvm. It looks like `llc` doesn't accept `-Os`; that `-O2` and `optsize` is how we get `-Os` in `llc`. TIL.


================
Comment at: llvm/test/CodeGen/X86/jcc-indirect-thunk-kernel.ll:5
+
+define dso_local void @foo(void()** %something) #0 {
+; CHECK-LABEL: foo:
----------------
joaomoreira wrote:
> nickdesaulniers wrote:
> > I think if you remove one layer of indirection then you might be able to drop the load instruction below?
> This is another weird one -- without the indirection, the jcc pattern is not generated, because the conditional (JE) is generated before the assignment of the function address to r11 and such assignment only happens if the conditional is met. With the indirection, the load takes the function address from memory and already places it on r11 before the condition is tested, what later makes room for fusing the pattern JE; JMP into JNE.
It looks like those 2 cases diverge during

Control Flow Optimizer (branch-folder)

due to block placement.  Not particularly sure why, but seems like it may be brittle.  I guess this test case must come from:

```
void foo (void (**something)(void)) {
  if (*something)
    (*something)();
}
```
I guess we'll keep the additional indirection for now...


================
Comment at: llvm/test/CodeGen/X86/jcc-indirect-thunk-kernel.ll:21
+if.then:
+  tail call void %0() #1
+  ret void
----------------
nickdesaulniers wrote:
> delete
This isn't done; just the context I highlighted has moved. Please remove the dangling reference to attribute `#1` from the tail call.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134915/new/

https://reviews.llvm.org/D134915