[Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop

[Weiming Zhao]

No problem. I will change it.

 

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?

 

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

 

From: Evan Cheng [mailto:evan.cheng at apple.com] 
Sent: Friday, March 14, 2014 12:55 PM
To: weimingz at codeaurora.org
Cc: Duncan P. N. Exon Smith; Tim.Northover at arm.com; llvm-commits at cs.uiuc.edu
Subject: Re: [Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop

 

The name isRegLive() is misleading. Why don't use call it isAnySubRegLive(),
which Duncan recommended? Otherwise, the revised patch looks fine to me.

 

Evan

 

On Mar 14, 2014, at 12:07 PM, Weiming Zhao <weimingz at codeaurora.org> wrote:





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)) {
<a.diff>_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140314/fcdfe737/attachment.html>