PR17326, PATCH
Nick Lewycky
nicholas at mxc.ca
Sat Dec 14 14:02:46 PST 2013
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
More information about the llvm-commits
mailing list