[PATCH] fix for PR16393: miscompile with struct byval

[Jakob Stoklund Olesen]

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.

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.

Thanks,
/jakob