<div style="font-family: arial, helvetica, sans-serif"><font size="2"><div class="gmail_quote">On Fri, Jun 15, 2012 at 6:21 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 bgcolor="#FFFFFF"><div class="im"><div><br></div><div>On Jun 15, 2012, at 6:07 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:<br>
<br></div><div></div><blockquote type="cite"><div><div style="font-family:arial,helvetica,sans-serif"><font size="2"><div class="gmail_quote">On Fri, Jun 15, 2012 at 6:06 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 bgcolor="#FFFFFF"><div>Chandler, please revert.</div></div></blockquote><div><br></div><div>Thanks, is there anything we can do to help fix the underlying issue?</div>
</div></font></div></div></blockquote><div><br></div></div><div>I understand the underlying issue, but I'm not familiar enough with the frame handling code to point you in the right direction this very moment. Essentially, the pops should be relative to the BP (RBX I believe), not the SP.</div>
<div class="im"><br><blockquote type="cite"><div><div style="font-family:arial,helvetica,sans-serif"><font size="2"><div class="gmail_quote"><div>Matt and I stared at it a bit, and it seems like there may be completely missing handling of the particular combination of features that this patch enables in a few places. If there is somewhere we can contribute patches to, let us know?</div>
</div></font></div></div></blockquote><div><br></div></div><div>I saw no CodeGen changes across the test-suite, but of course that's not your use case (i.e. the test-suite doesn't force stack alignment). Finding the bugs are very helpful.. And test cases! :). Beyond that I don't have any great suggestions.</div>
</div></blockquote><div><br></div><div>Ok, I've checked in a test case in r158656 to try to help keep these bits covered by tests.</div><div><br></div><div>I'm not terribly satisfied with the test case, as it's fairly sensitive to changes to any part of this stack, but hopefully it'll be easy enough to update for whatever the new logic is, and catch the issue when doing so.</div>
<div><br></div><div>The testcase looks like this:</div><div><br></div><div><font face="courier new, monospace">define i64 @g(i32 %i) {</font></div><div><font face="courier new, monospace">entry:</font></div><div><font face="courier new, monospace"> %0 = alloca i8, i32 %i</font></div>
<div><font face="courier new, monospace"> call void @llvm.memset.p0i8.i32(...)</font></div><div><font face="courier new, monospace"> %call = call i32 @f(...)</font></div><div><font face="courier new, monospace"> %conv = sext i32 %call to i64</font></div>
<div><font face="courier new, monospace"> ret i64 %conv</font></div><div><font face="courier new, monospace">}</font></div><div><br></div><div>Which when run through 'llc -force-stack-align -stack-alignment=32' (the best way I could think of to ensure an absurd about of stack re-alignment took place...) has asm like so after r158087:</div>
<div><br></div><div><pre class="sunburst" style="font-family:'Bitstream Vera Sans Mono',Monaco,Consolas,'Courier New',monospace;font-size:12px;line-height:15.600000381469727px;margin-top:0em"><span class="meta meta_paragraph meta_paragraph_text">g: # @g
# BB#0: # %entry
</span> <span class="meta meta_paragraph meta_paragraph_text">pushl %ebp
movl %esp, %ebp
andl $-32, %esp
pushl %ebx
pushl %esi
subl $24, %esp
movl %esp, %ebx
movl 8(%ebx), %eax
leal 31(%eax), %ecx
andl $-32, %ecx
movl %esp, %esi
subl %ecx, %esi
movl %esi, %esp
subl $32, %esp
movl %eax, 8(%esp)
movl %esi, (%esp)
movl $0, 4(%esp)
calll memset
addl $32, %esp
subl $32, %esp
movl %esi, (%esp)
calll f
addl $32, %esp
movl %eax, %edx
sarl $31, %edx
addl $24, %esp
popl %esi
popl %ebx
movl %ebp, %esp
popl %ebp
ret</span></pre></div><div><br></div><div>The bug is the 'addl $24, %esp' before the 'popl's. It has to reload from %ebp before it can pop any saved registers due to the alloca.</div><div><br></div>
<div>After reverting this patch, the same command produces:</div><div><br></div><div><pre class="sunburst" style="font-family:'Bitstream Vera Sans Mono',Monaco,Consolas,'Courier New',monospace;font-size:12px;line-height:15.600000381469727px;margin-top:0em">
<span class="meta meta_paragraph meta_paragraph_text">g: # @g
# BB#0: # %entry
</span> <span class="meta meta_paragraph meta_paragraph_text">pushl %ebp
movl %esp, %ebp
pushl %esi
subl $20, %esp
movl 8(%ebp), %eax
leal 31(%eax), %ecx
andl $-32, %ecx
movl %esp, %esi
subl %ecx, %esi
movl %esi, %esp
subl $32, %esp
movl %eax, 8(%esp)
movl %esi, (%esp)
movl $0, 4(%esp)
calll memset
addl $32, %esp
subl $32, %esp
movl %esi, (%esp)
calll f
addl $32, %esp
movl %eax, %edx
sarl $31, %edx
leal -4(%ebp), %esp
popl %esi
popl %ebp
ret</span></pre>Here the 'leal' off of %ebp restores the frame pointer correctly before popping registers.</div><div><br></div><div>Hope this is enough to help track down what the fix should be... Let me know if not.</div>
<div><br></div><div>Also, if you have any better ideas about how to write a test case for this, by all means chime in on my commit... I'm not really thrilled with the regression test strategy here.</div></div></font></div>