<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">The name isRegLive() is misleading. Why don't use call it isAnySubRegLive(), which Duncan recommended? Otherwise, the revised patch looks fine to me.<div><div><br></div><div>Evan</div><div><br><div><div>On Mar 14, 2014, at 12:07 PM, Weiming Zhao <<a href="mailto:weimingz@codeaurora.org">weimingz@codeaurora.org</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">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">http://llvm-reviews.chandlerc.com/D3085</a><span class="Apple-converted-space"> </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">mailto:dexonsmith@apple.com</a>]<span class="Apple-converted-space"> </span><br>Sent: Thursday, March 13, 2014 10:46 PM<br>To:<span class="Apple-converted-space"> </span><a href="mailto:weimingz@codeaurora.org">weimingz@codeaurora.org</a><br>Cc:<span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu">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">weimingz@codeaurora.org</a>> wrote:<br><br><blockquote type="cite">Hi,<br><br>This patch fixes the issue in<span class="Apple-converted-space"> </span><br><a href="http://llvm.org/bugs/show_bug.cgi?id=19136">http://llvm.org/bugs/show_bug.cgi?id=19136</a><br>There might be two ways to do it: One is to fix<br></blockquote>tryFoldSPUpdateIntoPushPop() to let it check livness of subregs.<br><blockquote type="cite">The other ways is to fix MBB->computeRegisterLiveness().<br><br>The patch uses the first way.<br><br>Please help to review.<br></blockquote><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><span><a.diff></span>_______________________________________________<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">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br></div></div></body></html>