[Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop

[Weiming Zhao]

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?

Thanks,
Weiming

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
b/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
&MF,
     // 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
fatal
       // for our transformation. GPR pops do, so we should just keep
looking.
       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
function:

    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>