[PATCH] fix for PR16393: miscompile with struct byval

[Jim Grosbach]

On Jul 8, 2013, at 11:49 AM, Manman Ren <mren at apple.com> wrote:

> 
> 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?

I haven’t followed closely enough to know all the recent details, but I don’t like the idea of having a property on MachineBasicBlock. I much prefer the pseudo-instruction to that.

My only hesitation about teaching PEI to do a CFG walk is that it’s adding a non-trivial bit of complexity, especially in the presence of crazy CFGs with computed-gotos and such in them. In theory, those shouldn’t interact with stack adjustments, but in practice?

The only bit I feel strongly about is that I agree we should have something that tries to verify those three invariants.

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