[PATCH] fix for PR16393: miscompile with struct byval

[Stepan Dyatkovskiy]

Hi Manman,
sorry for latency.

About MachineBasicBlock::StackAdjustment. There is no copy 
implementation for MachineBasicBlock at all.
Talking about transformations, I've checked what happens with 
MachineBasicBlock::IsLandingPad and MachineBasicBlock::Alignment fields. 
There is no default copy behaviour either. It seems (99.9%), these 
values are never copied.
In new patch I've updated MachineBasicBlock::print with StackAdjustment 
printing.
Note, that patch also contains additional test-case. I've wrote it from 
scratch (so its pretty short). In comparison with original one it causes 
llc to crash.

-Stepan.

Manman Ren wrote:
>
> On Jul 3, 2013, at 9:05 AM, Stepan Dyatkovskiy wrote:
>
>> Hi Manman,
>>
>> After some learning what exactly happens. IMHO, your decision is the *only* way (that couldn't be called too hackish or insane :-)).
> Have you tried other solutions?
>>
>> Though, may be we add SpAdj just as property for MachineBasicBlock class?
> Compared to using a pseudo SET_SP_ADJ, adding SPAdj as a property for MBB at least solves the issue of making sure SPAdj is available at the
> beginning of a basic block.
>
> I am not so sure whether it works when we have transformations on MBB.
> We also need to reset SPAdj when we try to eliminate call frame pseudo instructions in calculateCallsInformation.
>
> Thanks,
> Manman
>
>>
>> I've modified your patch, please find it in attachment.
>>
>> -Stepan.
>>
>> Manman Ren wrote:
>>>
>>> For a call site with struct byval, we will generate a sequence of instructions: STACKDOWN, STRUCT_BYVAL and STACKUP.
>>> STRUCT_BYVAL later on is expanded to a for loop. This caused issue in PEI when trying to eliminate frame indices.
>>>
>>> BB1:
>>>    STACKDOWN
>>> BB2:
>>>    memcpy
>>> BB3:
>>>    STACKUP
>>>
>>> PEI currently assumes SPAdj is 0 at the beginning of a basic block, but this is not true for BB2 or BB3. The proposed patch tries
>>> to fix the problem by inserting SET_SP_ADJ in BB2 and BB3:
>>> BB1:
>>>    STACKDOWN
>>> BB2:
>>>    SET_SP_ADJ
>>>    memcpy
>>> BB3:
>>>    SET_SP_ADJ
>>>    STACKUP
>>>
>>> A target-independent opcode SET_SP_ADJ is introduced, and pseudo instruction STRUCT_BYVAL is modified to take one extra
>>> argument (the amount of SP Adjustment). When we are expanding STRUCT_BYVAL to a for loop, we add SET_SP_ADJ to BB2 and BB3.
>>>
>>> PEI is also modified to handle SET_SP_ADJ. To make sure SET_SP_ADJ is always before any spill code that references frame indices,
>>> SET_SP_ADJ is treated as a label.
>>>
>>> SET_SP_ADJ can be useful for other targets when STACKDOWN and STACKUP are not paired up in the same basic block.
>>>
>>> Comments and other suggestions are welcome.
>>>
>>> Thanks,
>>> Manman
>>>
>>
>> <2013-06-03-byval-spadj.patch>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr16393-2016-07-08.patch
Type: text/x-diff
Size: 39749 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130708/862bafb5/attachment.patch>