[Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop

[Saleem Abdulrasool]

[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<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/20140314/4a1ad799/attachment.html>