[Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop

Weiming Zhao weimingz at codeaurora.org
Fri Mar 14 12:07:48 PDT 2014

Hi Duncan,

Thanks for the comment. You're right. I refactored the code. I upload it to
http://llvm-reviews.chandlerc.com/D3085 as well.

Hi Tim,

Could you help to review the logic?


Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation

-----Original Message-----
From: Duncan P. N. Exon Smith [mailto:dexonsmith at apple.com] 
Sent: Thursday, March 13, 2014 10:46 PM
To: weimingz at codeaurora.org
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop

On 2014 Mar 13, at 16:14, Weiming Zhao <weimingz at codeaurora.org> wrote:

> Hi,
> This patch fixes the issue in 
> http://llvm.org/bugs/show_bug.cgi?id=19136
> There might be two ways to do it: One is to fix
tryFoldSPUpdateIntoPushPop() to let it check livness of subregs.
> The other ways is to fix MBB->computeRegisterLiveness().
> The patch uses the first way.
> Please help to review.

diff --git a/lib/Target/ARM/ARMBaseInstrInfo.cpp
index 1aa146d..8a30d3c 100644
--- a/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ b/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -2160,9 +2160,14 @@ bool llvm::tryFoldSPUpdateIntoPushPop(MachineFunction
     // registers live within the function we might clobber a return value
     // register; the other way a register can be live here is if it's
     // callee-saved.
-    if (isCalleeSavedRegister(CurReg, CSRegs) ||
-        MBB->computeRegisterLiveness(TRI, CurReg, MI) !=
-            MachineBasicBlock::LQR_Dead) {
+    bool IsRegLive = false;
+    for (MCSubRegIterator Subreg(CurReg, TRI, /* IncludeSelf */true);
+         Subreg.isValid() && !IsRegLive; ++Subreg)
+      if (MBB->computeRegisterLiveness(TRI, *Subreg, MI) !=
+           MachineBasicBlock::LQR_Dead)
+        IsRegLive = true;
+    if (isCalleeSavedRegister(CurReg, CSRegs) || IsRegLive) {
       // VFP pops don't allow holes in the register list, so any skip is
       // for our transformation. GPR pops do, so we should just keep
       if (IsVFPPushPop)

I'm not an expert here, but your change looks unnecessarily bad for compile
time.  You now call computeRegisterLiveness() even when you don't use the
result.  You can refactor by moving the calculation into a static

    if (isCalleeSavedRegister(CurReg, CSRegs) ||
        isAnySubRegLive(MBB, CurReg, TRI)) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: a.diff
Type: application/octet-stream
Size: 3928 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140314/500bf03b/attachment.obj>

More information about the llvm-commits mailing list