<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 17 October 2014 13:00, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Fri, Oct 17, 2014 at 12:56 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On 17 October 2014 12:17, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">In fact this was PR7272.<br></blockquote><div><br></div></span><div>PR7272 is the other way around?</div><div><br></div><div><div>define void @foo(i32* %x) {</div><div> tail call void @bar(i32* byval %x) ;; bad tail from PR7272</div><div> ret void</div><div>}</div></div><div><br></div><div><div><div>define void @foo(i32* byval %x) {</div><div> tail call void @bar(i32* %x) ;; okay tail from this patch</div><div> ret void</div><div>}</div></div></div></div></div></div></blockquote><div><br></div></span><div>This is precisely the case when it's not safe to tail call. You're capturing the address of something allocated on the current stack frame and passing it down, which you can't do.</div></div></div></div></blockquote><div><br></div><div>Maybe I'm misunderstanding, but I really think this is fine. Specifically I claim your error is on the clause "allocated on the current stack frame". It's not allocated on the current stack frame, it's allocated on the stack frame one above us.</div><div><br></div><div>declare void @bar(i32*)<br></div><div><div><br></div><div>define void @foo(i32* byval %x) {</div><div> call void @bar(i32* %x)</div><div> ret void</div><div>}</div><div><br></div><div>define void @test(i32* %y) {</div><div> call void @foo(i32* byval %y)</div><div> ret void</div><div>}</div></div><div><br></div><div><div>foo: # @foo</div><div> pushq %rax<br></div><div> leaq 16(%rsp), %rdi<br></div><div> callq bar</div><div> popq %rax</div><div> retq</div></div><div><br></div><div><div>test: # @test</div><div> pushq %rax<br></div><div> movl (%rdi), %eax<br></div><div> movl %eax, (%rsp)</div><div> callq foo</div><div> popq %rax</div><div> retq</div></div><div><br></div><div>Let's see that again with a larger value being passed.</div><div><br></div><div><div>%large = type { [33 x i8] }</div><div><br></div><div>declare void @bar(%large*)</div><div><br></div><div>define void @foo(%large* byval %x) {</div><div> call void @bar(%large* %x)</div><div> ret void</div><div>}</div><div><br></div><div>define void @test(%large* %y) {</div><div> call void @foo(%large* byval %y)</div><div> ret void</div><div>}</div></div><div><br></div><div><div>foo: # @foo</div><div> pushq %rax<br></div><div> leaq 16(%rsp), %rdi<br></div><div> callq bar</div><div> popq %rax</div><div> retq</div><div><br></div><div>test: # @test</div><div> subq $40, %rsp<br></div><div> movb 32(%rdi), %al<br></div><div> movb %al, 32(%rsp)</div><div> movq 24(%rdi), %rax</div><div> movq %rax, 24(%rsp)</div><div> movq 16(%rdi), %rax</div><div> movq %rax, 16(%rsp)</div><div> movq (%rdi), %rax</div><div> movq 8(%rdi), %rcx</div><div> movq %rcx, 8(%rsp)</div><div> movq %rax, (%rsp)</div><div> callq foo</div><div> addq $40, %rsp</div><div> retq</div></div><div><br></div><div>Nice big stack adjustment in test, not in foo. Passing a byval argument is always, as far as I can tell, in the caller's stack not the callee's stack. foo is free to tail call that argument down to bar. It's equivalent to having an alloca in test, then foo can still tail call bar (we get that case right today).</div><div><br></div><div>There might be other ABIs which but byvals in the callee's stack. I don't know.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>The PR7272 case doesn't have that problem. It could hypothetically be lowered correctly, and IMO should be rejected by the backend instead of the middle end.<br></div></div></div></div></blockquote><div><br></div><div>It's hard for the backend to redo the analysis to determine whether any byval arguments flow into tail calls. I think if we do this in the middle, the backend would have to simply not do tail calls in the case where there function has a byval argument.</div><div><br></div><div>Nick</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>At least I think so?</div><div><div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div>On 17 October 2014 15:14, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
> Is this actually safe?<br>
><br>
> The caller will deallocate the stack and then pass a pointer to that<br>
> area to the callee...<br>
><br>
> On 15 October 2014 23:27, Saleem Abdulrasool <<a href="mailto:compnerd@compnerd.org" target="_blank">compnerd@compnerd.org</a>> wrote:<br>
>> Author: compnerd<br>
>> Date: Wed Oct 15 22:27:30 2014<br>
>> New Revision: 219899<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=219899&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=219899&view=rev</a><br>
>> Log:<br>
>> TRE: make TRE a bit more aggressive<br>
>><br>
>> Make tail recursion elimination a bit more aggressive. This allows us to get<br>
>> tail recursion on functions that are just branches to a different function. The<br>
>> fact that the function takes a byval argument does not restrict it from being<br>
>> optimised into just a tail call.<br>
>><br>
>> Added:<br>
>> llvm/trunk/test/Transforms/TailCallElim/byval.ll<br>
>> Modified:<br>
>> llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp<br>
>> llvm/trunk/test/Transforms/Inline/byval-tail-call.ll<br>
>> llvm/trunk/test/Transforms/TailCallElim/basic.ll<br>
>><br>
>> Modified: llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp?rev=219899&r1=219898&r2=219899&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp?rev=219899&r1=219898&r2=219899&view=diff</a><br>
>> ==============================================================================<br>
>> --- llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp (original)<br>
>> +++ llvm/trunk/lib/Transforms/Scalar/TailRecursionElimination.cpp Wed Oct 15 22:27:30 2014<br>
>> @@ -249,12 +249,7 @@ bool TailCallElim::markTails(Function &F<br>
>> return false;<br>
>> AllCallsAreTailCalls = true;<br>
>><br>
>> - // The local stack holds all alloca instructions and all byval arguments.<br>
>> AllocaDerivedValueTracker Tracker;<br>
>> - for (Argument &Arg : F.args()) {<br>
>> - if (Arg.hasByValAttr())<br>
>> - Tracker.walk(&Arg);<br>
>> - }<br>
>> for (auto &BB : F) {<br>
>> for (auto &I : BB)<br>
>> if (AllocaInst *AI = dyn_cast<AllocaInst>(&I))<br>
>> @@ -310,9 +305,8 @@ bool TailCallElim::markTails(Function &F<br>
>> for (auto &Arg : CI->arg_operands()) {<br>
>> if (isa<Constant>(Arg.getUser()))<br>
>> continue;<br>
>> - if (Argument *A = dyn_cast<Argument>(Arg.getUser()))<br>
>> - if (!A->hasByValAttr())<br>
>> - continue;<br>
>> + if (isa<Argument>(Arg.getUser()))<br>
>> + continue;<br>
>> SafeToTail = false;<br>
>> break;<br>
>> }<br>
>><br>
>> Modified: llvm/trunk/test/Transforms/Inline/byval-tail-call.ll<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/byval-tail-call.ll?rev=219899&r1=219898&r2=219899&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/byval-tail-call.ll?rev=219899&r1=219898&r2=219899&view=diff</a><br>
>> ==============================================================================<br>
>> --- llvm/trunk/test/Transforms/Inline/byval-tail-call.ll (original)<br>
>> +++ llvm/trunk/test/Transforms/Inline/byval-tail-call.ll Wed Oct 15 22:27:30 2014<br>
>> @@ -27,10 +27,11 @@ define internal void @qux(i32* byval %x)<br>
>> tail call void @ext(i32* null)<br>
>> ret void<br>
>> }<br>
>> +<br>
>> define void @frob(i32* %x) {<br>
>> ; CHECK-LABEL: define void @frob(<br>
>> ; CHECK: alloca i32<br>
>> -; CHECK: {{^ *}}call void @ext(<br>
>> +; CHECK: {{^ *}}tail call void @ext(<br>
>> ; CHECK: tail call void @ext(i32* null)<br>
>> ; CHECK: ret void<br>
>> tail call void @qux(i32* byval %x)<br>
>><br>
>> Modified: llvm/trunk/test/Transforms/TailCallElim/basic.ll<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/TailCallElim/basic.ll?rev=219899&r1=219898&r2=219899&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/TailCallElim/basic.ll?rev=219899&r1=219898&r2=219899&view=diff</a><br>
>> ==============================================================================<br>
>> --- llvm/trunk/test/Transforms/TailCallElim/basic.ll (original)<br>
>> +++ llvm/trunk/test/Transforms/TailCallElim/basic.ll Wed Oct 15 22:27:30 2014<br>
>> @@ -147,7 +147,7 @@ cond_false:<br>
>> ; Don't tail call if a byval arg is captured.<br>
>> define void @test9(i32* byval %a) {<br>
>> ; CHECK-LABEL: define void @test9(<br>
>> -; CHECK: {{^ *}}call void @use(<br>
>> +; CHECK: {{^ *}}tail call void @use(<br>
>> call void @use(i32* %a)<br>
>> ret void<br>
>> }<br>
>><br>
>> Added: llvm/trunk/test/Transforms/TailCallElim/byval.ll<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/TailCallElim/byval.ll?rev=219899&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/TailCallElim/byval.ll?rev=219899&view=auto</a><br>
>> ==============================================================================<br>
>> --- llvm/trunk/test/Transforms/TailCallElim/byval.ll (added)<br>
>> +++ llvm/trunk/test/Transforms/TailCallElim/byval.ll Wed Oct 15 22:27:30 2014<br>
>> @@ -0,0 +1,34 @@<br>
>> +; RUN: opt -mtriple i386 -Os -S %s -o - | FileCheck %s<br>
>> +; RUN: opt -mtriple x86_64 -Os -S %s -o - | FileCheck %s<br>
>> +; RUN: opt -mtriple armv7 -Os -S %s -o - | FileCheck %s<br>
>> +<br>
>> +%struct.D16 = type { [16 x double] }<br>
>> +<br>
>> +declare void @_Z2OpP3D16PKS_S2_(%struct.D16*, %struct.D16*, %struct.D16*)<br>
>> +<br>
>> +define void @_Z7TestRefRK3D16S1_(%struct.D16* noalias sret %agg.result, %struct.D16* %RHS, %struct.D16* %LHS) {<br>
>> + %1 = alloca %struct.D16*, align 8<br>
>> + %2 = alloca %struct.D16*, align 8<br>
>> + store %struct.D16* %RHS, %struct.D16** %1, align 8<br>
>> + store %struct.D16* %LHS, %struct.D16** %2, align 8<br>
>> + %3 = load %struct.D16** %1, align 8<br>
>> + %4 = load %struct.D16** %2, align 8<br>
>> + call void @_Z2OpP3D16PKS_S2_(%struct.D16* %agg.result, %struct.D16* %3, %struct.D16* %4)<br>
>> + ret void<br>
>> +}<br>
>> +<br>
>> +; CHECK: define void @_Z7TestRefRK3D16S1_({{.*}}) {<br>
>> +; CHECK: tail call void @_Z2OpP3D16PKS_S2_(%struct.D16* %agg.result, %struct.D16* %RHS, %struct.D16* %LHS)<br>
>> +; CHECK: ret void<br>
>> +; CHECK: }<br>
>> +<br>
>> +define void @_Z7TestVal3D16S_(%struct.D16* noalias sret %agg.result, %struct.D16* byval align 8 %RHS, %struct.D16* byval align 8 %LHS) {<br>
>> + call void @_Z2OpP3D16PKS_S2_(%struct.D16* %agg.result, %struct.D16* %RHS, %struct.D16* %LHS)<br>
>> + ret void<br>
>> +}<br>
>> +<br>
>> +; CHECK: define void @_Z7TestVal3D16S_({{.*}}) {<br>
>> +; CHECK: tail call void @_Z2OpP3D16PKS_S2_(%struct.D16* %agg.result, %struct.D16* %RHS, %struct.D16* %LHS)<br>
>> +; CHECK: ret void<br>
>> +; CHECK: }<br>
>> +<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>