[Patch] [ARM] Fix bug in tryFoldSPUpdateIntoPushPop

[weimingz at codeaurora.org]

Hi Salleem,

The reason is I not very sure about fixing ComputeRegisterLiveness().
In the function:
01177       if (Analysis.Defines)
01178         // Outputs happen after inputs so they take precedence if
both are
01179         // present.
01180         return Analysis.DefinesDead ? LQR_Dead : LQR_Live;
01181
01182       if (Analysis.Kills || Analysis.Clobbers)
01183         // Register killed, so isn't live.
01184         return LQR_Dead;

Since s0 is defined, so for AnalyzePhysReg(D0,TRI).Clobbers = true, so it
returns LQR_Dead. Should we return LRQ_Live for Analysis.Clobber? I'm not
very sure.

Thanks,
Weiming



> [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
>