[PATCH] D21395: Fix for Bug 28144

[vivek pandya via llvm-commits]

On Wed, Jun 22, 2016 at 11:25 PM, Matthias Braun <matze at braunis.de> wrote:

>
> > On Jun 22, 2016, at 1:58 AM, Vivek Pandya via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >
> > vivekvpandya added a comment.
> >
> > @mehdi_amini , at MatzeB
> > Here is my analysis:
> >
> >  BitVector UsedPhysRegMask = MRI->getUsedPhysRegsMask();
> >    for (unsigned i = 0; i < RegMaskSize; ++i)
> >      RegMask[i] &= ~UsedPhysRegMask[i];
> >
> > The above code is buggy because UsedPhysRegMask is indexed by register
> no where
> > as RegMask is vector integers in which each bit represenet status of one
> register.
> > I am sorry for that, I assumed that in LLVM all regmask are following
> same representation.
> >
> >  // Code snippet 1 (CS1)
> >    for (unsigned PReg = 1, PRegE = TRI->getNumRegs(); PReg < PRegE;
> ++PReg)
> >      if (!MRI->reg_nodbg_empty(PReg) && MRI->isPhysRegUsed(PReg))
> >        markRegClobbered(TRI, &RegMask[0], PReg);
> >
> >
> >
> >  // Code snippet 2 (CS2)
> >    for (unsigned PReg = 1, PRegE = TRI->getNumRegs(); PReg < PRegE;
> ++PReg)
> >      if (MRI->isPhysRegUsed(PReg))
> >        markRegClobbered(TRI, &RegMask[0], PReg);
> >
> > Now actaully CS2 is sufficient and correct code for calculating regmask.
> And CS1
> > is having a very subtle bug which caused incorrect regmask (such as not
> marking
> > R11 as clobbered). Now why CS2 is correct and sufficient (I think
> @MatzeB has
> > pointed this fact earlier) it is because MRI::isPhysRegUsed actaully uses
> > UsedPhysRegMask so no need to consider that details again and
> MRI::isPhysRegUsed
> > also uses reg_nodbg_empty.
> >
> > Now what is the problem with CS1, due to !MRI->reg_nodbg_empty(PReg)
> condition
> > fails when MF is not using PReg and MRI->isPhysRegUsed(PReg) would never
> execute.
> > thus generating wrong regmask. So why it happens so. Consider following
> definition
> > of reg_nodbg_empty and reg_nodbg_begin, it uses getRegUseDefListHead and
> if given
> > register is not used in side a MF then it will return true (I think it
> should not
> > Isn't reg_nodbg_empty return false if Reg is not used in MF at all?)
> > and due to that !MRI->reg_nodbg_empty(PReg) will be false.
> >
> >  /// reg_nodbg_empty - Return true if the only instructions using or
> defining
> >  /// Reg are Debug instructions.
> >    bool reg_nodbg_empty(unsigned RegNo) const {
> >       return reg_nodbg_begin(RegNo) == reg_nodbg_end();
> >    }
> >
> >    reg_nodbg_iterator reg_nodbg_begin(unsigned RegNo) const {
> >       return reg_nodbg_iterator(getRegUseDefListHead(RegNo));
> >     }
> >
> > What are your thoughts? Have I missed any thing?
>
> I don't understand your concerns about reg_nodbg_empty().
>
But as I am initializing RegMask as all 1 i.e all preserved and if I keep
!reg_nodbg_empty() check then registers like R11 is kept as preserved in
RegMask which is wrong information and causes bug. So I think we don't need
this check. About the behavior of  reg_nodbg_empty() i.e if reg is not used
inside MF then it returns true that is just out of curiosity and I feel
that regs which are not used in side MF should not be returned true in
reg_nodbg_empty().

>
> As far as isPhysRegUsed() goes: This function has additional code that
> does not consider the results of tail calls as used, which is okay if you
> want to figure out what registers need to be saved inside a function, but
> to determine a regmask for outside callers this is not fine. So I think
> using isPhysRegUsed() is wrong for your use case and possible the source of
> the new bugs.
>
So Do you feel isPhysRegModified() is more suitable for this?

-Vivek

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