[llvm-commits] PATCH: Change X86 function prologue/epilogue in case the stack is realigned

[Alexey Samsonov]

r160248, thanks!

On Sat, Jul 14, 2012 at 3:57 AM, Chad Rosier <mcrosier at apple.com> wrote:

> LGTM!  Thanks for working on this, Alexey.
>
>  Chad
>
> On Jul 13, 2012, at 9:24 AM, Alexey Samsonov wrote:
>
>
> On Fri, Jul 13, 2012 at 2:52 PM, Alexey Samsonov <samsonov at google.com>wrote:
>
>> Hi, Chad!
>>
>> On Fri, Jul 13, 2012 at 12:03 AM, Chad Rosier <mcrosier at apple.com> wrote:
>>
>>>
>>> On Jul 12, 2012, at 12:54 PM, Alexey Samsonov wrote:
>>>
>>> Thanks for your comments! I'll fix the patch and sent the updated
>>> version tomorrow.
>>>
>>> Fixed. (updated patch attached and uploaded to
>> http://codereview.appspot.com/6352105/). More comments below.
>>
>
> And one more update - we should reflect the change in order of stack
> alignment / pushing registers,
> while calculating the frame size, so that stack would indeed be
> properly aligned.
> http://codereview.appspot.com/6352105/
>
>
>>
>>
>>> I also don't know a case where pushing the registers before the stack
>>> alignment may cause any troubles (I just checked that gcc sometimes behaves
>>> this way, but it is not Windows, and no xmm registers were involved there).
>>> Do you think that we may get away with documenting
>>> this concern in comments?
>>>
>>>
>>> We should definitely leave a comment in the event that this change
>>> causes subtle problems.
>>>
>>
>>  Okay, so I found the following comment in
>> X86FrameLowering::spillCalleeSavedRegisters
>>   // Make XMM regs spilled. X86 does not have ability of push/pop XMM.
>>   // It can be done by spilling XMMs to stack frame.
>>   // Note that only Win64 ABI might spill XMMs.
>>
>> So, the XMM regs are saved in stack slots, and aren't pushed/popped. I've
>> added a reference to this code
>> and commented our concern in emitPrologue() anyway.
>>
>>
>>>
>>> Jakob commented on a trick that ARM does in a similar situation.
>>>  Basically, there is an unaligned spill area and an aligned spill area (for
>>> performance reasons).  I haven't looked at the code, however.  This is
>>> mostly just a side comment, but maybe you could glean some insight and
>>> possible pitfalls by looking at the ARM implementation?
>>>
>>
>> Thanks for your advice about the ARM code - the comments there say that
>> it's a bad idea to emit consecutive instructions
>> mov %rbp, %rsp
>> sub $size_of_pushed_regs, %rsp
>> as the interrupt may happen between these two instructions, which can
>> spoil the callee-saved registers. I've substituted it with
>> lea $size_of_pushed_regs(%rbp), %rsp
>> (see the expected asm in tests) and simplified code in emitEpilogue() a
>> bit.
>>
>>
>>>
>>>  Chad
>>>
>>>
>>> On Thu, Jul 12, 2012 at 10:04 PM, Chad Rosier <mcrosier at apple.com>wrote:
>>>
>>>> Alexey,
>>>> Is there a case where we would ever need the stack realigned before the
>>>> pushes?  This came to mind prior to my comments below, but I didn't think
>>>> it was possible.  Then Eric dropped by my office and made a similar
>>>> comment.  That got me wondering, so I briefly chatted with Jakob.  He
>>>> pointed out that in Windows there are vector callee-saved registers (xmm
>>>> regs).  The only issue would be if these need to be saved and the alignment
>>>> is below 16 bytes.  We weren't sure if this could ever happen, but just as
>>>> a precaution I though I would comment.
>>>>
>>>
>>>
>>>>
>>>>  Chad
>>>>
>>>>
>>>> On Jul 12, 2012, at 9:53 AM, Chad Rosier wrote:
>>>>
>>>>
>>>> On Jul 12, 2012, at 9:02 AM, Alexey Samsonov wrote:
>>>>
>>>> Hi!
>>>>
>>>> This patch is intended to fix the exception handling in case of
>>>> realigned stack (see more details in recent discussion with mcrosier at on r160002).
>>>>
>>>>
>>>> This looks to be a better solution then the prior one we discussed, so
>>>> I'm all for it.
>>>> In fact, it will improve the performance in the case of stack
>>>> realignment + VLAs because we don't restore the SP twice.
>>>>
>>>> Before this change, the prologue and epilogue looks like this:
>>>> push %rbp
>>>> mov %rsp, %rbp
>>>> and $alignment %rsp   <--- align stack
>>>> push %reg1   <----- save regs on stack
>>>> push %reg2
>>>> sub $size %rsp
>>>> <...>
>>>> add $size %rsp
>>>> pop %reg2  <------- restore regs
>>>> pop %reg1
>>>> mov %rbp, %rsp <--- restore rsp
>>>> pop %rbp
>>>>
>>>> The problem is: the register values are incorrectly restored in
>>>> exception handling - emiited call frame information gives offsets of
>>>> registers from frame pointer (%rbp),
>>>> which is wrong, as their actual locations depend on the alignment
>>>> result.
>>>>
>>>> I suggest to modify the prologue in the following way:
>>>> push %rbp
>>>> mov %rsp, %rbp
>>>> push %reg1   <----- save regs on stack
>>>> push %reg2
>>>> and $alignment %rsp   <--- align stack
>>>> sub $size %rsp
>>>> <...>
>>>> mov %rbp, %rsp
>>>> sub $total_size_of_saved_regs, %rsp
>>>> pop %reg2  <------- restore regs
>>>> pop %reg1
>>>> pop %rbp
>>>>
>>>> In this way the offsets from %rbp will be valid, and the EH won't be
>>>> broken. (you may also see the emiited asm in the tests).
>>>>
>>>>
>>>> Nice!
>>>>
>>>> Just a few comments on the patch:
>>>> 1. For the new test case, pr11468.ll, would you mind adding CHECKs for
>>>> the epilogue?  It just gives a more complete picture of what's going on.
>>>>
>>>> 2. In dynamic-allocas-VLAs.ll
>>>>
>>>> Because this is being removed from t4:
>>>> -; CHECK: addq $[[STACKADJ]], %rsp
>>>>
>>>> Would you mind simplifying the earlier check:
>>>>  ; CHECK: subq $[[STACKADJ:[0-9]+]], %rsp
>>>>
>>>> to this:
>>>>  ; CHECK: subq ${{[0-9]+}}, %rsp
>>>>
>>>> And for this CHECK:
>>>> +; CHECK: addq ${{[0-9]*}}, %rsp ## imm = 0xFFFFFFF0
>>>>
>>>> Use a + instead of a *; I assume we should see at least one digit.
>>>>  Also, remove the ## comment unless you feel it's informative.
>>>>
>>>> The above applies to the t7 test case as well.
>>>>
>>>> And for t9 please removed the ## comments unless you feel it's
>>>> informative.
>>>>
>>>> Looks good otherwise.
>>>>
>>>>  Chad
>>>>
>>>> Code review: http://codereview.appspot.com/6352105/
>>>>
>>>> --
>>>> Alexey Samsonov, MSK
>>>>
>>>> <zdiff.pr11468>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Alexey Samsonov, MSK
>>>
>>>
>>>
>>
>>
>> --
>> Alexey Samsonov, MSK
>>
>>
>
>
> --
> Alexey Samsonov, MSK
>
> <issue6352105_5003.diff>
>
>
>


-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120716/4224208d/attachment.html>