[PATCH] fix for PR16393: miscompile with struct byval

[Manman Ren]

On Jul 8, 2013, at 11:02 AM, Jakob Stoklund Olesen <jolesen at apple.com> wrote:

> 
> On Jul 2, 2013, at 5:19 PM, Manman Ren <mren at apple.com> 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.
> 
> Hi Manman,
> 
> I am a bit worried that these solutions are too fragile. Every pass that changes the machine CFG needs to know how to preserve the SET_SP_ADJ instructions or MBB::StackAdjustment property. This is likely to break because SET_SP_ADJ is used so rarely.
Yes it is a bit fragile.

> 
> How about teaching PEI that STACKDOWN/STACKUP pairs don’t have to be in the same basic block? PEI could verify that:
> 
> - On every path through the CFG, a STACKDOWN <n> is always followed by a STACKUP <n> instruction.
> - STACKDOWN/STACKUP pairs are never nested.
> - Stack adjustments are identical on all CFG edges to a merge point.
> 
> This could be implemented by having PEI traverse the CFG in a DFS order, keeping track of the stack state at the end of every basic block on the DFS stack.

Theoretically, this should work. But there are some corner cases in PEI which may break this.
Jim, do you have any opinion?

Thanks,
Manman

> 
> Thanks,
> /jakob

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130708/c720b0e9/attachment.html>