<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 14 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:"\@SimSun";
panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p
{mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Should we commit the fix and put a TODO or FIXME in the code?<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Thanks,<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Weiming<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><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'><o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><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"'> Saleem Abdulrasool [mailto:compnerd@compnerd.org] <br><b>Sent:</b> Friday, March 14, 2014 7:02 PM<br><b>To:</b> weimingz@codeaurora.org<br><b>Cc:</b> Evan Cheng; llvm-commits; Tim Northover<br><b>Subject:</b> Re: [Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop<o:p></o:p></span></p><p class=MsoNormal><o:p> </o:p></p><div><div><div><p class=MsoNormal>[Correct Tim's email address]<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>On Fri, Mar 14, 2014 at 1:23 PM, Weiming Zhao <<a href="mailto:weimingz@codeaurora.org" target="_blank">weimingz@codeaurora.org</a>> wrote:<o:p></o:p></p><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>No problem. I will change it.</span><o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><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><o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>The (original) logic seems right to me, but Tim should really take a look at it.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>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.<o:p></o:p></p></div><div><p class=MsoNormal> <o:p></o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in'><div><div><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><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><o:p></o:p></p></div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> </span><o:p></o:p></p></div><div><div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in'><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><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><o:p></o:p></p><div><div><p class=MsoNormal><br><b>Subject:</b> Re: [Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop<o:p></o:p></p></div></div></div></div><div><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'> <o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>The name isRegLive() is misleading. Why don't use call it isAnySubRegLive(), which Duncan recommended? Otherwise, the revised patch looks fine to me.<o:p></o:p></p><div><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'> <o:p></o:p></p></div><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>Evan<o:p></o:p></p></div><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'> <o:p></o:p></p><div><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>On Mar 14, 2014, at 12:07 PM, Weiming Zhao <<a href="mailto:weimingz@codeaurora.org" target="_blank">weimingz@codeaurora.org</a>> wrote:<o:p></o:p></p></div><p class=MsoNormal style='mso-margin-top-alt:auto;margin-bottom:12.0pt'><o:p> </o:p></p><div><p class=MsoNormal style='mso-margin-top-alt:auto;margin-bottom:12.0pt'><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> 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>] <br>Sent: Thursday, March 13, 2014 10:46 PM<br>To: <a href="mailto:weimingz@codeaurora.org" target="_blank">weimingz@codeaurora.org</a><br>Cc: <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></span><o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><span style='font-size:9.0pt'>Hi,<br><br>This patch fixes the issue in <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</span><o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;margin-bottom:12.0pt'><span style='font-size:9.0pt'>tryFoldSPUpdateIntoPushPop() to let it check livness of subregs.</span><o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><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.</span><o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><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></span><o:p></o:p></p></div></div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'> <o:p></o:p></p></div></div></div></div></div><p class=MsoNormal style='margin-bottom:12.0pt'><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><o:p></o:p></p></blockquote></div><p class=MsoNormal><br><br clear=all><o:p></o:p></p><div><p class=MsoNormal><o:p> </o:p></p></div><p class=MsoNormal>-- <br>Saleem Abdulrasool<br>compnerd (at) compnerd (dot) org <o:p></o:p></p></div></div></div></body></html>