[Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop

Weiming Zhao weimingz at codeaurora.org
Wed Mar 19 10:24:00 PDT 2014


Should we commit the fix and put a TODO or FIXME in the code?

 

Thanks,

Weiming

 

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

 

From: Saleem Abdulrasool [mailto:compnerd at compnerd.org] 
Sent: Friday, March 14, 2014 7:02 PM
To: weimingz at codeaurora.org
Cc: Evan Cheng; llvm-commits; Tim Northover
Subject: Re: [Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop

 

[Correct Tim's email address]

 

On Fri, Mar 14, 2014 at 1:23 PM, Weiming Zhao <weimingz at codeaurora.org> wrote:

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?

 

The (original) logic seems right to me, but Tim should really take a look at it.

 

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.

 

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

 


_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits





 

-- 
Saleem Abdulrasool
compnerd (at) compnerd (dot) org 

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


More information about the llvm-commits mailing list