PR17326, PATCH

[Stepan Dyatkovskiy]

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: 2957 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131216/158f220b/attachment.patch>