PR17326, PATCH

[Stepan Dyatkovskiy]

ping
Stepan Dyatkovskiy wrote:
> Well any thoughts?
> As addition to my previous post. Shall we do memcpy? Yes, in this case
> (tricky bitcast) need to store things into memory. Partly it is memcpy,
> partly regs contents. I didn't get idea about
> extractelement/insertelement. Would you explain, please?
>
> -Stepan.
>
> Stepan Dyatkovskiy wrote:
>> 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
>>>>
>>>
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>