<br><div class="gmail_quote">On Fri, Jul 13, 2012 at 2:52 PM, Alexey Samsonov <span dir="ltr"><<a href="mailto:samsonov@google.com" target="_blank">samsonov@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi, Chad!<br><br><div class="gmail_quote"><div class="im">On Fri, Jul 13, 2012 at 12:03 AM, Chad Rosier <span dir="ltr"><<a href="mailto:mcrosier@apple.com" target="_blank">mcrosier@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div><div>On Jul 12, 2012, at 12:54 PM, Alexey Samsonov wrote:</div><br><blockquote type="cite">Thanks for your comments! I'll fix the patch and sent the updated version tomorrow.</blockquote>
</div></div></div></blockquote></div><div>Fixed. (updated patch attached and uploaded to <a href="http://codereview.appspot.com/6352105/" target="_blank">http://codereview.appspot.com/6352105/</a>). More comments below.</div>
</div></blockquote><div><br></div><div>And one more update - we should reflect the change in order of stack alignment / pushing registers,</div><div>while calculating the frame size, so that stack would indeed be properly aligned.</div>
<div><a href="http://codereview.appspot.com/6352105/">http://codereview.appspot.com/6352105/</a></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote">
<div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><br><blockquote type="cite"><div><div>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</div>
<div>this concern in comments?</div></div></blockquote><div><br></div></div><div>We should definitely leave a comment in the event that this change causes subtle problems.</div></div></div></blockquote><div><br></div></div>
<div>
Okay, so I found the following comment in X86FrameLowering::spillCalleeSavedRegisters</div><div><div> // Make XMM regs spilled. X86 does not have ability of push/pop XMM.</div><div> // It can be done by spilling XMMs to stack frame.</div>
<div> // Note that only Win64 ABI might spill XMMs.</div></div><div><br></div><div>So, the XMM regs are saved in stack slots, and aren't pushed/popped. I've added a reference to this code</div><div>and commented our concern in emitPrologue() anyway.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br></div><div>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?</div>
</div></div></blockquote><div><br></div></div><div>Thanks for your advice about the ARM code - the comments there say that it's a bad idea to emit consecutive instructions</div><div>mov %rbp, %rsp</div><div>sub $size_of_pushed_regs, %rsp</div>
<div>as the interrupt may happen between these two instructions, which can spoil the callee-saved registers. I've substituted it with</div><div>lea $size_of_pushed_regs(%rbp), %rsp</div><div>(see the expected asm in tests) and simplified code in emitEpilogue() a bit.</div>
<div><div class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span><font color="#888888"><div><br></div><div> Chad</div>
</font></span><div><div><br><blockquote type="cite"><div><div><br></div><div class="gmail_quote">On Thu, Jul 12, 2012 at 10:04 PM, Chad Rosier <span dir="ltr"><<a href="mailto:mcrosier@apple.com" target="_blank">mcrosier@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Alexey,<div>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.</div>
</div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><span><font color="#888888"><div><br></div>
</font></span><div><span><font color="#888888"> Chad</font></span><div><div><br><div><br></div><div><div><div>On Jul 12, 2012, at 9:53 AM, Chad Rosier wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word">
<br><div><div>On Jul 12, 2012, at 9:02 AM, Alexey Samsonov wrote:</div><br><blockquote type="cite"><div>Hi!</div><div><br></div><div>This patch is intended to fix the exception handling in case of realigned stack (see more details in recent discussion with mcrosier@ on r160002).</div>
</blockquote><div><br></div><div>This looks to be a better solution then the prior one we discussed, so I'm all for it. </div><div>In fact, it will improve the performance in the case of stack realignment + VLAs because we don't restore the SP twice.</div>
<br><blockquote type="cite"><div>Before this change, the prologue and epilogue looks like this:</div>
<div>push %rbp</div><div>mov %rsp, %rbp</div><div>and $alignment %rsp <--- align stack</div><div>push %reg1 <----- save regs on stack</div><div>push %reg2</div><div>sub $size %rsp</div><div><...></div><div>
add $size %rsp</div><div>pop %reg2 <------- restore regs</div><div>pop %reg1</div><div>mov %rbp, %rsp <--- restore rsp</div><div>pop %rbp</div><div><br></div><div>The problem is: the register values are incorrectly restored in exception handling - emiited call frame information gives offsets of registers from frame pointer (%rbp),</div>
<div>which is wrong, as their actual locations depend on the alignment result.</div><div><br></div><div>I suggest to modify the prologue in the following way:</div><div>push %rbp</div><div><div>mov %rsp, %rbp</div><div>push %reg1 <----- save regs on stack</div>
<div>push %reg2</div></div><div>and $alignment %rsp <--- align stack</div><div><div>sub $size %rsp</div><div><...></div><div>mov %rbp, %rsp</div><div>sub $total_size_of_saved_regs, %rsp</div><div>pop %reg2 <------- restore regs</div>
<div>pop %reg1</div><div>pop %rbp</div></div><div><br></div><div>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).</div></blockquote><div><br>
</div><div>Nice!</div><div><br></div><div>Just a few comments on the patch:</div><div>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.</div>
<div><br></div><div>2. In dynamic-allocas-VLAs.ll</div><div><div><br></div><div>Because this is being removed from t4:</div><div>-; CHECK: addq $[[STACKADJ]], %rsp</div><div><br></div><div>Would you mind simplifying the earlier check:</div>
<div> ; CHECK: subq $[[STACKADJ:[0-9]+]], %rsp</div><div><br></div><div>to this:</div><div> ; CHECK: subq ${{[0-9]+}}, %rsp</div><div><br></div><div>And for this CHECK:</div><div>+; CHECK: addq ${{[0-9]*}}, %rsp ## imm = 0xFFFFFFF0</div>
<div><br></div><div>Use a + instead of a *; I assume we should see at least one digit. Also, remove the ## comment unless you feel it's informative.</div><div><br></div><div>The above applies to the t7 test case as well.</div>
<div><br></div><div>And for t9 please removed the ## comments unless you feel it's informative.</div><div><br></div><div>Looks good otherwise.</div><div><br></div><div> Chad</div><div><br></div></div><blockquote type="cite">
Code review: <a href="http://codereview.appspot.com/6352105/" target="_blank">http://codereview.appspot.com/6352105/</a><br clear="all">
<div><br></div>-- <br><div>Alexey Samsonov, MSK</div><br>
<span><zdiff.pr11468></span></blockquote></div><br></div></blockquote></div><br></div></div></div></div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div><br>
</div>
</blockquote></div></div></div><br></div>
</blockquote></div></div></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div><br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div><br>