[PATCH] D21395: Fix for Bug 28144

[Matthias Braun via llvm-commits]

> On Jun 22, 2016, at 11:05 AM, vivek pandya via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 
> 
> On Wed, Jun 22, 2016 at 11:25 PM, Matthias Braun <matze at braunis.de <mailto:matze at braunis.de>> wrote:
> 
> > On Jun 22, 2016, at 1:58 AM, Vivek Pandya via llvm-commits <llvm-commits at lists.llvm.org <mailto: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().
I still don't follow. reg_nobdg_empty() tests whether the list of MachineOperands is empty, so if a register is not used, there are no machine operands and the list is empty and we should return true.

> 
> 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?
No, that suffers from the same problem. I believe you need to write your own. It should look similar to isPhysRegModified but not do the isNoReturnDef() check.

- Matthias

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