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

[Peter Collingbourne]

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?

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
> 

-- 
Peter