[Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop

[Evan Cheng]

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/79bc1312/attachment.html>