<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">[Correct Tim's email address]</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Fri, Mar 14, 2014 at 1:23 PM, Weiming Zhao <span dir="ltr"><<a href="mailto:weimingz@codeaurora.org" target="_blank">weimingz@codeaurora.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">No problem. I will change it.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Another interesting thing is that it uses s16 instead of other callee saved vfp regs. I tried using –O3 and removing “minsize”, still the same. Any ideas?</span></p>
</div></blockquote><div><br></div><div>The (original) logic seems right to me, but Tim should really take a look at it.</div><div><br></div><div>I am however curious as to why you took this approach rather than fix computeRegisterLiveness? It seems that the latter (possibly more involved) is the better approach. This feels like it could be easy to re-introduce elsewhere if you were to try to check register liveness for any reason.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div class=""><div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></p>
</div><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p></div><div><div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Evan Cheng [mailto:<a href="mailto:evan.cheng@apple.com" target="_blank">evan.cheng@apple.com</a>] <br>
<b>Sent:</b> Friday, March 14, 2014 12:55 PM<br><b>To:</b> <a href="mailto:weimingz@codeaurora.org" target="_blank">weimingz@codeaurora.org</a><br><b>Cc:</b> Duncan P. N. Exon Smith; <a href="mailto:Tim.Northover@arm.com" target="_blank">Tim.Northover@arm.com</a>; <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a></span></p>
<div><div class="h5"><br><b>Subject:</b> Re: [Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop<u></u><u></u></div></div><p></p></div></div><div><div class="h5"><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">
The name isRegLive() is misleading. Why don't use call it isAnySubRegLive(), which Duncan recommended? Otherwise, the revised patch looks fine to me.<u></u><u></u></p><div><div><p class="MsoNormal"><u></u> <u></u></p>
</div><div><p class="MsoNormal">Evan<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p><div><div><p class="MsoNormal">On Mar 14, 2014, at 12:07 PM, Weiming Zhao <<a href="mailto:weimingz@codeaurora.org" target="_blank">weimingz@codeaurora.org</a>> wrote:<u></u><u></u></p>
</div><p class="MsoNormal"><br><br><u></u><u></u></p><div><p class="MsoNormal"><span style="font-size:9.0pt">Hi Duncan,<br><br>Thanks for the comment. You're right. I refactored the code. I upload it to<br><a href="http://llvm-reviews.chandlerc.com/D3085" target="_blank">http://llvm-reviews.chandlerc.com/D3085</a><span> </span>as well.<br>
<br>Hi Tim,<br><br>Could you help to review the logic?<br><br>Thanks,<br>Weiming<br><br>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by<br>The Linux Foundation<br><br><br>-----Original Message-----<br>
From: Duncan P. N. Exon Smith [<a href="mailto:dexonsmith@apple.com" target="_blank">mailto:dexonsmith@apple.com</a>]<span> </span><br>Sent: Thursday, March 13, 2014 10:46 PM<br>To:<span> </span><a href="mailto:weimingz@codeaurora.org" target="_blank">weimingz@codeaurora.org</a><br>
Cc:<span> </span><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>Subject: Re: [Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop<br><br><br>On 2014 Mar 13, at 16:14, Weiming Zhao <<a href="mailto:weimingz@codeaurora.org" target="_blank">weimingz@codeaurora.org</a>> wrote:<br>
<br><br><u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:9.0pt">Hi,<br><br>This patch fixes the issue in<span> </span><br><a href="http://llvm.org/bugs/show_bug.cgi?id=19136" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=19136</a><br>
There might be two ways to do it: One is to fix<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:9.0pt">tryFoldSPUpdateIntoPushPop() to let it check livness of subregs.<br><br><u></u><u></u></span></p><p class="MsoNormal">
<span style="font-size:9.0pt">The other ways is to fix MBB->computeRegisterLiveness().<br><br>The patch uses the first way.<br><br>Please help to review.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:9.0pt"><br>
diff --git a/lib/Target/ARM/ARMBaseInstrInfo.cpp<br>b/lib/Target/ARM/ARMBaseInstrInfo.cpp<br>index 1aa146d..8a30d3c 100644<br>--- a/lib/Target/ARM/ARMBaseInstrInfo.cpp<br>+++ b/lib/Target/ARM/ARMBaseInstrInfo.cpp<br>@@ -2160,9 +2160,14 @@ bool llvm::tryFoldSPUpdateIntoPushPop(MachineFunction<br>
&MF,<br> // registers live within the function we might clobber a return value<br> // register; the other way a register can be live here is if it's<br> // callee-saved.<br>- if (isCalleeSavedRegister(CurReg, CSRegs) ||<br>
- MBB->computeRegisterLiveness(TRI, CurReg, MI) !=<br>- MachineBasicBlock::LQR_Dead) {<br>+ bool IsRegLive = false;<br>+ for (MCSubRegIterator Subreg(CurReg, TRI, /* IncludeSelf */true);<br>+ Subreg.isValid() && !IsRegLive; ++Subreg)<br>
+ if (MBB->computeRegisterLiveness(TRI, *Subreg, MI) !=<br>+ MachineBasicBlock::LQR_Dead)<br>+ IsRegLive = true;<br>+<br>+ if (isCalleeSavedRegister(CurReg, CSRegs) || IsRegLive) {<br> // VFP pops don't allow holes in the register list, so any skip is<br>
fatal<br> // for our transformation. GPR pops do, so we should just keep<br>looking.<br> if (IsVFPPushPop)<br><br>I'm not an expert here, but your change looks unnecessarily bad for compile<br>time. You now call computeRegisterLiveness() even when you don't use the<br>
result. You can refactor by moving the calculation into a static<br>function:<br><br> if (isCalleeSavedRegister(CurReg, CSRegs) ||<br> isAnySubRegLive(MBB, CurReg, TRI)) {<br><a.diff>_______________________________________________<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><u></u><u></u></span></p>
</div></div><p class="MsoNormal"><u></u> <u></u></p></div></div></div></div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">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>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br>Saleem Abdulrasool<br>compnerd (at) compnerd (dot) org
</div></div>