[llvm] r293727 - [IPSCCP] Teach how to not propagate return values of naked functions.

Wed Feb 1 18:43:53 PST 2017

On Wed, Feb 1, 2017 at 5:48 PM, Davide Italiano <davide at freebsd.org> wrote:

> On Wed, Feb 1, 2017 at 5:36 PM, Sean Silva <chisophugis at gmail.com> wrote:
> >
> >
> > On Tue, Jan 31, 2017 at 5:01 PM, Davide Italiano via llvm-commits
> > <llvm-commits at lists.llvm.org> wrote:
> >>
> >> Author: davide
> >> Date: Tue Jan 31 19:01:22 2017
> >> New Revision: 293727
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=293727&view=rev
> >> Log:
> >> [IPSCCP] Teach how to not propagate return values of naked functions.
> >>
> >> Differential Revision:  https://reviews.llvm.org/D29360
> >>
> >> Added:
> >>     llvm/trunk/test/Transforms/IPConstantProp/naked-return.ll
> >> Modified:
> >>     llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
> >>
> >> Modified: llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/Scalar/SCCP.cpp?rev=293727&r1=293726&r2=293727&view=diff
> >>
> >> ============================================================
> ==================
> >> --- llvm/trunk/lib/Transforms/Scalar/SCCP.cpp (original)
> >> +++ llvm/trunk/lib/Transforms/Scalar/SCCP.cpp Tue Jan 31 19:01:22 2017
> >> @@ -1712,7 +1712,10 @@ static bool runIPSCCP(Module &M, const D
> >>
> >>      // If this is an exact definition of this function, then we can
> >> propagate
> >>      // information about its result into callsites of it.
> >> -    if (F.hasExactDefinition())
> >> +    // Don't touch naked functions. They may contain asm returning a
> >> +    // value we don't see, so we may end up interprocedurally
> propagating
> >> +    // the return value incorrectly.
> >> +    if (F.hasExactDefinition() && !F.hasFnAttribute(Attribute::Naked))
> >>        Solver.AddTrackedFunction(&F);
> >>
> >>      // If this function only has direct calls that we can see, we can
> >> track its
> >>
> >> Added: llvm/trunk/test/Transforms/IPConstantProp/naked-return.ll
> >> URL:
> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> Transforms/IPConstantProp/naked-return.ll?rev=293727&view=auto
> >>
> >> ============================================================
> ==================
> >> --- llvm/trunk/test/Transforms/IPConstantProp/naked-return.ll (added)
> >> +++ llvm/trunk/test/Transforms/IPConstantProp/naked-return.ll Tue Jan
> 31
> >> 19:01:22 2017
> >> @@ -0,0 +1,28 @@
> >> +; RUN: opt -ipsccp -S %s | FileCheck %s
> >> +
> >> +target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
> >> +target triple = "i686-pc-windows-msvc19.0.24215"
> >> +
> >> +define i32 @dipsy(i32, i32) local_unnamed_addr #0 {
> >> +BasicBlock0:
> >> +  call void asm "\0D\0Apushl %ebp\0D\0Amovl 8(%esp),%eax\0D\0Amovl
> >> 12(%esp), %ebp\0D\0Acalll *%eax\0D\0Apopl %ebp\0D\0Aretl\0D\0A", ""()
> >> +  ret i32 0
> >
> >
> > If this asm is going to return, shouldn't we have `unreachable` instead
> of
> > `ret i32 0`? That would naturally be correct I think without requiring
> > special handling for naked functions. I.e., the verifier should ensure
> that
> > naked functions always end with `unreachable`.
> >
>
> That would require some other code to make sure we don't propagate the
> unreachable to the caller (which apparently PruneEH currently does). WDYT?
>

It sounds like the missing thing is (at least) being able to mark an inline
asm as returning (hard to do without making it a terminator?). More
generally, marking inline asm's as affecting control flow is also a
longstanding issue preventing us from implementing "asm goto". So this
might be pretty nontrivial.
Maybe we can model it as the inline asm having a dummy out param which is
the thing that is returned? (or marking the inline asm such that the
optimizer thinks it might throw an exception, do a longjmp, or any other
side effect that will conservatively model the asm's behavior of returning
from the current function)

It just seems wrong for the special case check to be for naked functions
since the issue seems to be more about modeling what an inline asm will do.

-- Sean Silva

>
> $ ./opt -O3 naked-return.ll -opt-bisect-limit=85 -S
>
> [...]
> BISECT: running pass (83) Aggressive Dead Code Elimination on function
> (tinkywinky)
> BISECT: running pass (84) Simplify the CFG on function (tinkywinky)
> BISECT: running pass (85) Combine redundant instructions on function
> (tinkywinky)
> [...]
>
> ; Function Attrs: naked noreturn nounwind
> define i32 @dipsy(i32, i32) local_unnamed_addr #0 {
> BasicBlock0:
>   tail call void asm "\0D\0Apushl %ebp\0D\0Amovl
> 8(%esp),%eax\0D\0Amovl 12(%esp), %ebp\0D\0Acalll *%eax\0D\0Apopl
> %ebp\0D\0Aretl\0D\0A", ""() #3
>   unreachable
> }
> [...]
> define void @patatino(i32, i32, i32) local_unnamed_addr #2 {
> bb:
>   %3 = tail call i32 @dipsy(i32 %0, i32 %1) #4
>   tail call void @tinkywinky(i32 %3, i32 %2, i32 %1) #4
>   ret void
> }
>
> and
> $ ./opt -O3 naked-return.ll -opt-bisect-limit=86 -S
> [...]
> BISECT: running pass (83) Aggressive Dead Code Elimination on function
> (tinkywinky)
> BISECT: running pass (84) Simplify the CFG on function (tinkywinky)
> BISECT: running pass (85) Combine redundant instructions on function
> (tinkywinky)
> BISECT: running pass (86) Remove unused exception handling info on SCC
> (patatino)
> BISECT: NOT running pass (87) Function Integration/Inlining on SCC
> (patatino)
> [...]
>
> define void @patatino(i32, i32, i32) local_unnamed_addr #2 {
> bb:
>   %3 = tail call i32 @dipsy(i32 %0, i32 %1) #4
>   unreachable
> }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170201/c65bb259/attachment.html>