[llvm] r192302 - Fix a bug in Dead Argument Elimination.

[Nick Lewycky]

Nick Lewycky wrote:
> Peter Collingbourne wrote:
>> Hi Shuxin/John,
>>
>> Apologies for the (very) late review, but I don't understand this change.
>> The fundamental assumption we make in LLVM is that if mayBeOverridden()
>> returns false, the semantics of the function cannot be changed by other
>> compilation units. This assumption is used by the inliner among other
>> optimization passes. What is special about dead argument elimination that
>> invalidates this assumption?
>
> Suppose you have a callee that does:
>
> %A = load i32 %ptr
> ret void
>
> You can inline that just fine, but you can't necessarily deadargelim
> %ptr if the callee is provided by a different TU which is compiled at -O0.
>
> In other words, there's something special about the linker

Something special about the inliner, as opposed to other IPOs. Sorry.

, and it's the
> fact that it copies the whole body. Any IPO that doesn't do that must
> consider whether the def'n could come from a different TU with less
> optimization applied. This requires a linkage which is both strong
> enough to inline through but two definitions does not cause a linker error.
>
> Shuxin, I don't think the test makes the underlying problem clear. It
> shows a thing about the behaviour, but I don't think anyone will
> understand why that's necessary if they look at the test a year from
> now. Could you improve that?
>
>>
>> Thanks,
>> Peter
>>
>> On Wed, Oct 09, 2013 at 05:21:44PM -0000, Shuxin Yang wrote:
>>> Author: shuxin_yang
>>> Date: Wed Oct 9 12:21:44 2013
>>> New Revision: 192302
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=192302&view=rev
>>> Log:
>>> Fix a bug in Dead Argument Elimination.
>>>
>>> If a function seen at compile time is not necessarily the one linked to
>>> the binary being built, it is illegal to change the actual arguments
>>> passing to it.
>>>
>>> e.g.
>>> --------------------------
>>> void foo(int lol) {
>>> // foo() has linkage satisifying isWeakForLinker()
>>> // "lol" is not used at all.
>>> }
>>>
>>> void bar(int lo2) {
>>> // xform to foo(undef) is illegal, as compiler dose not know which
>>> // instance of foo() will be linked to the the binary being built.
>>> foo(lol2);
>>> }
>>> -----------------------------
>>>
>>> Such functions can be captured by isWeakForLinker(). NOTE that
>>> mayBeOverridden() is insufficient for this purpose as it dosen't include
>>> linkage types like AvailableExternallyLinkage and LinkOnceODRLinkage.
>>> Take link_odr* as an example, it indicates a set of *EQUIVALENT* globals
>>> that can be merged at link-time. However, the semantic of
>>> *EQUIVALENT*-functions includes parameters. Changing parameters breaks
>>> the assumption.
>>>
>>> Thank John McCall for help, especially for the explanation of subtle
>>> difference between linkage types.
>>>
>>> rdar://11546243
>>>
>>> Added:
>>> llvm/trunk/test/Transforms/DeadArgElim/linkage.ll
>>> Modified:
>>> llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp
>>>
>>> Modified: llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp?rev=192302&r1=192301&r2=192302&view=diff
>>>
>>> ==============================================================================
>>>
>>> --- llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp Wed Oct
>>> 9 12:21:44 2013
>>> @@ -357,6 +357,19 @@ bool DAE::RemoveDeadArgumentsFromCallers
>>> if (Fn.hasLocalLinkage()&& !Fn.getFunctionType()->isVarArg())
>>> return false;
>>>
>>> + // If a function seen at compile time is not necessarily the one
>>> linked to
>>> + // the binary being built, it is illegal to change the actual
>>> arguments
>>> + // passing to it. These functions can be captured by
>>> isWeakForLinker().
>>> + // *NOTE* that mayBeOverridden() is insufficient for this purpose
>>> as it
>>> + // dosen't include linkage types like AvailableExternallyLinkage and
>>> + // LinkOnceODRLinkage. Take link_odr* as an example, it indicates a
>>> set of
>>> + // *EQUIVALENT* globals that can be merged at link-time. However, the
>>> + // semantic of *EQUIVALENT*-functions includes parameters. Changing
>>> + // parameters breaks the assumption.
>>> + //
>>> + if (Fn.isWeakForLinker())
>>> + return false;
>>> +
>>> if (Fn.use_empty())
>>> return false;
>>>
>>>
>>> Added: llvm/trunk/test/Transforms/DeadArgElim/linkage.ll
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadArgElim/linkage.ll?rev=192302&view=auto
>>>
>>> ==============================================================================
>>>
>>> --- llvm/trunk/test/Transforms/DeadArgElim/linkage.ll (added)
>>> +++ llvm/trunk/test/Transforms/DeadArgElim/linkage.ll Wed Oct 9
>>> 12:21:44 2013
>>> @@ -0,0 +1,21 @@
>>> +; RUN: opt< %s -deadargelim -S | FileCheck %s
>>> +
>>> +; rdar://11546243
>>> +%struct.A = type { i8 }
>>> +
>>> +define available_externally void
>>> @_Z17externallyDefinedP1A(%struct.A* %a) {
>>> +entry:
>>> + call void @_Z3foov()
>>> + ret void
>>> +}
>>> +
>>> +declare void @_Z3foov()
>>> +
>>> +define void @_Z4testP1A(%struct.A* %a) {
>>> +; CHECK: @_Z4testP1A
>>> +; CHECK: @_Z17externallyDefinedP1A(%struct.A* %a)
>>> +
>>> +entry:
>>> + call void @_Z17externallyDefinedP1A(%struct.A* %a)
>>> + ret void
>>> +}
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>