[PATCH] fix for PR16393: miscompile with struct byval

[Stepan Dyatkovskiy]

Hi Manman,

> Have you tried other solutions?
Yes. I've tried several concepts, though some them seems to be ugly:
1. Try to mark somehow call-sequence-start BB, call-sequence-end-BB and call BB. Since (as we can see) in general it could be several basic blocks. In combination with your patch this approach gave me patch I presented in previous mail.

2. While scanning BBs in PEI::replaceFrameIndices, try to recognize copy loop. We know that original BB was expanded onto: head, loop and exit BBs, and we can treat with them as with single BB. It wasn't a good idea to do it straght in replaceFrameIndices, so ideally it should be one more backend hook method: something like getCallBBs()

3. Do the parameter copy *before* ADJSTACKDOWN node:
  COPY_STRUCT_BY_VAL -> ADJSTACKDOWN -> CALL -> ADJSTACKUP

> I am not so sure whether it works when we have transformations on MBB.
If any further tranformations are supposed, I think, we just should also update MBB clone implementation.

-Stepan.

03.07.2013, 23:15, "Manman Ren" <mren at apple.com>:
> 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>