PR17326, PATCH

[Nick Lewycky]

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