[PATCH] D21395: Fix for Bug 28144

[Hal Finkel via llvm-commits]

----- Original Message -----

> From: "Matthias Braun via llvm-commits" <llvm-commits at lists.llvm.org>
> To: "vivek pandya" <vivekvpandya at gmail.com>
> Cc: reviews+D21395+public+6520003f52704029 at reviews.llvm.org, "kv
> bhat" <kv.bhat at samsung.com>, llvm-commits at lists.llvm.org
> Sent: Wednesday, June 22, 2016 1:12:34 PM
> Subject: Re: [PATCH] D21395: Fix for Bug 28144

> > 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
> > >
> > 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().
> 
> 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.
Why not add a parameter to isPhysRegModified, false by default, to skip that check? 

-Hal 

> - Matthias

> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160622/29dd5826/attachment.html>