[llvm] r200717 - inalloca: Don't remove dead arguments in the presence of inalloca args

[Reid Kleckner]

On Mon, Feb 3, 2014 at 10:35 PM, Nick Lewycky <nicholas at mxc.ca> wrote:

> Reid Kleckner wrote:
>
>> Author: rnk
>> Date: Mon Feb  3 14:42:49 2014
>> New Revision: 200717
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=200717&view=rev
>> Log:
>> inalloca: Don't remove dead arguments in the presence of inalloca args
>>
>> It disturbs the layout of the parameters in memory and registers,
>> leading to problems in the backend.
>>
>
> :-(
>
>
>  The plan for optimizing internal inalloca functions going forward is to
>> essentially SROA the argument memory and demote any captured arguments
>> (things that aren't trivially written by a load or store) to an indirect
>> pointer to a static alloca.
>>
>> Modified:
>>      llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp
>>      llvm/trunk/test/Transforms/DeadArgElim/keepalive.ll
>>
>> Modified: llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/
>> DeadArgumentElimination.cpp?rev=200717&r1=200716&r2=200717&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp (original)
>> +++ llvm/trunk/lib/Transforms/IPO/DeadArgumentElimination.cpp Mon Feb  3
>> 14:42:49 2014
>> @@ -531,6 +531,13 @@ DAE::Liveness DAE::SurveyUses(const Valu
>>   // well as arguments to functions which have their "address taken".
>>   //
>>   void DAE::SurveyFunction(const Function&F) {
>> +  // Functions with inalloca parameters are expecting args in a
>> particular
>> +  // register and memory layout.
>> +  if (F.getAttributes().hasAttrSomewhere(Attribute::InAlloca)) {
>> +    MarkLive(F);
>>
>
> Hold on. Does it actually need to be live? Consider A(x) calls B(x) calls
> C(x). If C takes 'x' as inalloca and doesn't use it, B can pass in
> C(undef). If B doesn't take 'x' as inalloca, A doesn't need to pass B
> anything.


No, it doesn't.  We could actually do the transform in this case if we
changed the calling convention to something that doesn't have register
parameters.

 +    return;
>> +  }
>> +
>>     unsigned RetCount = NumRetVals(&F);
>>     // Assume all return values are dead
>>     typedef SmallVector<Liveness, 5>  RetVals;
>>
>> Modified: llvm/trunk/test/Transforms/DeadArgElim/keepalive.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
>> Transforms/DeadArgElim/keepalive.ll?rev=200717&r1=
>> 200716&r2=200717&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/test/Transforms/DeadArgElim/keepalive.ll (original)
>> +++ llvm/trunk/test/Transforms/DeadArgElim/keepalive.ll Mon Feb  3
>> 14:42:49 2014
>> @@ -28,4 +28,20 @@ define void @caller() {
>>           ret void
>>   }
>>
>> +; We can't remove 'this' here, as that would put argmem in ecx instead of
>> +; memory.
>> +define internal x86_thiscallcc i32 @unused_this(i32* %this, i32*
>> inalloca %argmem) {
>>
>
> If it isn't used, please it readnone nocapture instead. Replacing it with
> 'undef' at the visible callsites would be nice too.
>

That seems reasonable, but is it worth spending time on this instead of on
argument promotion for inalloca?  That seems like a better way to enable
DAE.


> Also, is this a representative case in the real world? Why do you have an
> internal x86_thiscallcc function instead of an internal fastcc function?


Well, I reduced the crash from tablegen, so apparently we don't apply
fastcc on everything internal?


>
>  +       %v = load i32* %argmem
>> +       ret i32 %v
>> +}
>> +; CHECK-LABEL: define internal x86_thiscallcc i32 @unused_this(i32*
>> %this, i32* inalloca %argmem)
>> +
>> +define i32 @caller2() {
>> +       %t = alloca i32
>> +       %m = alloca i32, inalloca
>> +       store i32 42, i32* %m
>> +       %v = call x86_thiscallcc i32 @unused_this(i32* %t, i32* inalloca
>> %m)
>> +       ret i32 %v
>> +}
>> +
>>   ; CHECK: attributes #0 = { nounwind }
>>
>>
>> _______________________________________________
>> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140203/334b9fd5/attachment.html>