[PATCH] D21395: Fix for Bug 28144

[vivek pandya via llvm-commits]

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

>
> On Jun 22, 2016, at 11:19 AM, Hal Finkel via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>
>
> ------------------------------
>
> *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?
>
> Works as well.
>
I think by default we should not skip that check (so that we don't break
any existing code) but when parameter is true we skip that check. I hope
that is what Hal meant.
-Vivek

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