PR17326, PATCH

[Stepan Dyatkovskiy]

Reattached patch (previous one a bit broken)
-Stepan
Stepan Dyatkovskiy wrote:
> Hi Nick,
> I have attached updated patch (tricky bitcast).
>
>  > What assembly do we actually generate for this? I expect we can't
>  > eliminate the memcpy. Do we do any better if we build up
>  > extractvalue/insertvalue pairs for each element?
> Yes. Once aggregates comes to us in registers, we need store them first.
> I'll explain.
> I didn't find way in llvm, of how to represent the same function
> argument as new aggregate. Only thing I found out it is strictly
> forbidden in general :-)
> The hole in current llvm rules - is possibility to bitcast pointers. So
> we need to get pointer to original aggregate contents somehow, and then
> bitcast it to any structure we wish. Since argument could be stored in
> registers as well (and we can't find out whether its in registers before
> ISelLowering), we need to store it somewhere first.
> Assertion I used here, is that aggregates supposed to be read-only in
> function body.
>
> Answering to you next question:
>  > What assembly do we actually generate for this?
> IR:
> define void @f1(%Aggr1) {
>    %2 = alloca %Aggr1
>    store %Aggr1 %0, %Aggr1* %2
>    %3 = bitcast %Aggr1* %2 to %Aggr0*
>    %4 = load %Aggr0* %3
>    tail call void @f0(%Aggr0 %4)
>    ret void
> }
>
> For arm-linux-gnueabi (that stores this structure in r0):
> f1:
> @ BB#0:
>      push.w    {r11, lr}
>      sub    sp, #8
>      str    r0, [sp, #4]
>      bl    f0
>      add    sp, #8
>      pop.w    {r11, pc}
>
>
>  > As for the tricky-bitcast patch:
>  >
>  > -    Args.push_back(createCast(Builder, (Value*)AI,
>  > FFTy->getParamType(i)));
>  > +    Value *Aggr = ((Value*)AI);
>  >
>  > Is that "Value *Aggr = AI.get();" ?
> I have not found "get" method in arg_iterator, nor in Argument class
> definition. What I meant - is just to represent function argument as
> "Value" for farther manipulations.
>
>  > Style issue: if you have braces on one side of an if/else, use braces on
>  > both sides.
> Sure. Simply overlooked. Fixed.
>
> -Stepan
>
> Nick Lewycky wrote:
>> Stepan Dyatkovskiy wrote:
>>> ping
>>> Stepan Dyatkovskiy wrote:
>>>> ping.
>>>> Stepan Dyatkovskiy wrote:
>>>>> ping
>>>>> Stepan Dyatkovskiy wrote:
>>>>>> Updated patches.
>>>>>>
>>>>>> Stepan Dyatkovskiy wrote:
>>>>>>> ping.
>>>>>>> Could somebody review that?
>>>>>>> -Stepan
>>>>>>> Peter Zotov wrote:
>>>>>>>> Stepan Dyatkovskiy писал 19.11.2013 18:33:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> Please consider two possible fixes.
>>>>>>>>>
>>>>>>>>> 1. pr17326-forbid-aggregates.patch. If we meet functions with
>>>>>>>>> aggregates as i-th parameter, treat them as equal only in case
>>>>>>>>> when it
>>>>>>>>> is exactly the same aggregate: LeftParamType == RightParamType.
>>>>>>>>>
>>>>>>>>> 2. pr17326-tricky-bitcast.patch. Hackish. If we meet functions
>>>>>>>>> with
>>>>>>>>> aggregates as i-th parameter, and we 100500% sure aggregates are
>>>>>>>>> identical, use kind of memcpy in thunk wrapper:
>>>>>>>>> store %AggrTyLeft %ThatAggregateParameter, %AggrTyLeft* %2
>>>>>>>>> %3 = bitcast %AggrTyLeft* %2 to %AggrTyRight*
>>>>>>>>> %4 = load %AggrTyRight* %3
>>>>>>>>> tail call void @f0(%AggrTyRight %4)
>>>>>>>>>
>>>>>>>>> IMHO, it is a seldom case. And its better to forbid it to merging.
>>
>> There's an important use case where we want to treat two aggregates, one
>> of which is equal to the other plus extra things appended to the end, as
>> the same. (Note: can growing a struct by adding elements change the
>> earlier parts of the struct? For instance, could it increase the type's
>> natural alignment?) This is to support frontends that do inheritance by
>> constructing a second copy of the base type plus the new fields in the
>> derived type. For example, %base = type { i32, i32 } and %derived = type
>> { i32, i32, i32 } should be the merged for a function that only accesses
>> the first two members. Obviously we can't do this if the third member is
>> used or the pointer is passed through a call site.
>>
>> For starters, the forbid-aggregates patch is obviously correct. Peter,
>> do you actually need to handle treating the aggregates the same across
>> function calls? Not pointers to aggregates, but passing the aggregates
>> themselves? Structs in SSA registers is always a little weird, it's
>> something we kinda discourage.
>>
>> As for the tricky-bitcast patch:
>>
>> -    Args.push_back(createCast(Builder, (Value*)AI,
>> FFTy->getParamType(i)));
>> +    Value *Aggr = ((Value*)AI);
>>
>> Is that "Value *Aggr = AI.get();" ?
>>
>> +      Value *AggrCopyPtr = Builder.CreateAlloca(SrcTy);
>> +      Builder.CreateStore(Aggr, AggrCopyPtr);
>> +      Type *DestPtrTy = DestTy->getPointerTo();
>> +      Value *AggrCopyDestPtr = Builder.CreateBitCast(AggrCopyPtr,
>> DestPtrTy);
>> +      Value *Dest = Builder.CreateLoad(AggrCopyDestPtr);
>> +      Args.push_back(Dest);
>>
>> What assembly do we actually generate for this? I expect we can't
>> eliminate the memcpy. Do we do any better if we build up
>> extractvalue/insertvalue pairs for each element?
>>
>> +    }
>> +    else
>> +      Args.push_back(createCast(Builder, (Value*)AI,
>> FFTy->getParamType(i)));
>>
>> Style issue: if you have braces on one side of an if/else, use braces on
>> both sides.
>>
>> Nick
>>
>>>>>>>>
>>>>>>>> Personally, I'm in favor of tricky-bitcast (perhaps done more
>>>>>>>> cleanly).
>>>>>>>> This was my intended use case for mergefunc in the first place.
>>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> 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
>>>>
>>>> _______________________________________________
>>>> 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 --------------
A non-text attachment was scrubbed...
Name: pr17326-tricky-bitcast-2013-12-16.patch
Type: text/x-diff
Size: 2963 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131217/772d9d7d/attachment.patch>