<div dir="ltr">Thank you for the review, Jakob. The patch was committed in r214580.<div><br></div><div>The existing set of inline-asm tests in inline-asm-fpstack.ll looks fine to me, but test cases for function calls might be missing. I'll see if I should add something for them.</div>
<div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 31, 2014 at 9:29 AM, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:stoklund@2pi.dk" target="_blank">stoklund@2pi.dk</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">Hi Akira,<div><br></div><div>This looks good to me. It's great to delete so much code!</div>
<div><br></div><div>I think we have pretty good test coverage for x87 code and inline asm, but you may want to take a second look at the existing tests to see of we missed any corner cases.</div><div><br></div><div>Thanks,</div>
<div>/jakob</div><div><br></div><div><div><div><div class="h5"><div>On Jul 30, 2014, at 1:53 PM, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:</div><br></div></div>
<blockquote type="cite"><div style="font-family:Optima-Regular;font-size:18px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div><div class="h5"><div dir="ltr">Updated and rebased patch attached.<div><br></div><div>- Trimmed the test case.</div><div>- Added comments explaining how the stackifier distinguishes between register operands with "f" constraints and those with single register class constraints ("t" or "u").</div>
<div>- Fixed a bug in FPS::handleSpecialFP where register operands with single class constraints were not rewritten correctly. Added a test case for the bug.</div><div>- Fixed bug in X86FastISel::FastLowerCall.</div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Fri, Jul 18, 2014 at 3:04 PM, Bob Wilson<span> </span><span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span><span> </span>wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">Ping (on behalf of Akira).<div>
<br></div><div>I’ve looked over the patch and it seems like a very nice improvement to the way we handle x87 registers. Unfortunately I have very little experience in this area and don’t feel qualified to review it.</div>
<div><br></div><div>Can someone else more familiar with x87 stuff please review it?</div><div><br><div><blockquote type="cite"><div><div><div>On Jul 1, 2014, at 4:55 PM, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:</div>
<br></div></div><div><div><div><div dir="ltr"><p style="margin:0px 0px 12px;padding:0px;border:0px;font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.850000381469727px">The x87 FP stackifier pass has a bug where the liveness of an ST register defined by an inline-asm instruction is not tracked precisely.</p>
<p style="margin:0px 0px 12px;padding:0px;border:0px;font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.850000381469727px">This causes the assertion on line 1365 to fail when an ST register defined by an inline-asm is live across another inline-asm, as shown in the following example:</p>
<div style="margin:0px;font-size:11px;font-family:Menlo"><span style="font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.850000381469727px">INLINEASM <es:frndint> [sideeffect] [attdialect], $0:[regdef], %ST0<imp-def,tied5>, $1:[reguse tiedto:$0], %ST0<tied3>, $2:[clobber], %EFLAGS<earlyclobber,imp-def,dead></span><br>
</div><p style="margin:0px 0px 12px;padding:0px;border:0px;font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.850000381469727px">INLINEASM <es:fldcw $0> [sideeffect] [mayload] [attdialect], $0:[mem], %EAX<undef>, 1, %noreg, 0, %noreg, $1:[clobber], %EFLAGS<earlyclobber,imp-def,dead><br>
%FP0<def> = COPY %ST0 // %ST0 is defined by the first inline-asm instruction.</p><p style="margin:0px 0px 12px;padding:0px;border:0px;font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.850000381469727px">
In order to fix this bug, the attached patch uses FP registers for function returns and inline-asm instructions where previously ST registers were being used and adds kill flags that are missing in FP register operands.</p>
<div style="margin:0px;padding:0px;border:0px;font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;font-size:13px;line-height:18.850000381469727px"><a>rdar://problem/16952634</a></div></div></div></div><span><x87stackifier1.patch></span>_______________________________________________<br>
llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></blockquote></div><br></div></div></blockquote></div><br></div></div></div></div><span><x87stackifier2.patch></span></div></blockquote></div><br></div></div></blockquote></div><br></div>