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

[Davide Italiano via llvm-commits]

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?

$ ./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
}