[PATCH] fix for PR16393: miscompile with struct byval
Stepan Dyatkovskiy
stpworld at narod.ru
Mon Jul 8 03:10:44 PDT 2013
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>
More information about the llvm-commits
mailing list