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

[Nick Lewycky]

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, 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
>>
>