[PATCH] D105807: [X86] pr51000 in-register struct return tailcalling

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a subscriber: xbolva00.
rnk added a comment.

Looks like there's a correctness issue on 32-bit x86.



================
Comment at: clang/test/CodeGenCXX/pr51000.cpp:1
+// RUN: %clang -S %s -o - -O2 -Xclang -triple=x86_64-linux | FileCheck %s --check-prefix=X86 --check-prefix=X86_64
+// RUN: %clang -S %s -o - -O2 -Xclang -triple=x86_64-linux-gnux32 | FileCheck %s --check-prefix=X86 --check-prefix=X86_32
----------------
This is an integration test between Clang and LLVM, which are generally discouraged. Your code change is to the x86 backend, so the LLVM CodeGen/X86 test should be sufficient. I was unable to find any documentation to point to support this discouragement. >_>


================
Comment at: llvm/test/CodeGen/X86/sibcall.ll:661
+;; WARNING: This testcase is UB, as the tail call is passed a pointer
+;; to the caller's frame.  It should not be marked tail call.
+
----------------
I dug into the history, and it seems @xbolva00 added this test in a previous attempt to do more TCO around sret. The test is from rG642ed40e57faa34ee3ae7ba0e32f75ff582d408b and the attempt was D46262.

Honestly, I think you can remove the test. I assumed it had some purpose, but I don't see any motivation for this UB in the review comments.


================
Comment at: llvm/test/CodeGen/X86/sibcall.ll:1026-1027
-; X86-NEXT:    movl %eax, (%esp)
-; X86-NEXT:    calll t22_f_sret at PLT
-; X86-NEXT:    addl $8, %esp
-; X86-NEXT:    retl
----------------
Hang on, this seems wrong. The stack adjustments aren't balanced.

I think we can't do TCO for 32-bit x86. It pops off the sret pointer. See this IR:
```
define ccc void @t22_f_sret(i32* sret(i32) %p) {
        store i32 0, i32* %p
        ret void
}
define ccc void @t22_non_sret_to_sret(i32* %agg.result) nounwind  {
        tail call ccc void @t22_f_sret(i32* noalias sret(i32) %agg.result) nounwind
        ret void
}
```
->
$ llc t.ll -o - -mtriple=i686-linux-gnu
```
t22_f_sret:                             # @t22_f_sret
        .cfi_startproc
# %bb.0:
        movl    4(%esp), %eax
        movl    $0, (%eax)
        retl    $4
.Lfunc_end0:
...
t22_non_sret_to_sret:                   # @t22_non_sret_to_sret
# %bb.0:
        subl    $12, %esp
        movl    16(%esp), %eax
        movl    %eax, (%esp)
        calll   t22_f_sret at PLT
        addl    $8, %esp
        retl
```

The `retl $4` instruction here is key, it adjusts ESP to pop off extra argument memory.


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

https://reviews.llvm.org/D105807