<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jul 12, 2012, at 9:02 AM, Alexey Samsonov wrote:</div><br class="Apple-interchange-newline"><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/">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></body></html>