>-----Original Message----- >From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of >Sanjoy Das via llvm-dev >Sent: Thursday, December 03, 2015 11:16 PM >To: Quentin Colombet <qcolombet at apple.com> >Cc: llvm-dev at lists.llvm.org >Subject: Re: [llvm-dev] analyzePhysReg question > >I think this is related to PR25033: >https://llvm.org/bugs/show_bug.cgi?id=25033#c9Yes, I agree this is very closely related. The code referenced in that PR if (Analysis.Kills || Analysis.Clobbers) // Register killed, so isn't live. return LQR_Dead; is incorrect I think. A clobber isn't a full def, and so cannot be assumed to be a kill. Some bits from the live-in value may pass through into the live-out value. Looking into the code for anaylzePhysReg, I think the code looks like it intends for Defines to truly mean Reg or a super-register of Reg is defined, not any overlapping Reg is defined. The code looks like this: bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(MOReg, Reg); ... if (IsRegOrSuperReg) { PRI.Defines = true; // Reg or a super-register is defined. if (!MO.isDead()) AllDefsDead = false; } I think the fundamental bug here is that the operands are swapped when passed into isSuperRegister. The definition of isSuperRegister is /// \brief Returns true if RegB is a super-register of RegA. bool isSuperRegister(unsigned RegA, unsigned RegB) const; so, it looks to me as if in the call to isSuperRegister, the parameters are swapped, and analyzePhysReg should really be asking whether the operand Reg (MOReg) is a super register of Reg, and the code should be: bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(Reg, MOReg);> >-- Sanjoy > >On Thu, Dec 3, 2015 at 5:39 PM, Quentin Colombet via llvm-dev ><llvm-dev at lists.llvm.org> wrote: >> >> On Dec 3, 2015, at 5:36 PM, Quentin Colombet via llvm-dev >> <llvm-dev at lists.llvm.org> wrote: >> >> >> On Dec 3, 2015, at 5:11 PM, Smith, Kevin B <kevin.b.smith at intel.com> >wrote: >> >> >> >> -----Original Message----- >> From: Quentin Colombet [mailto:qcolombet at apple.com] >> Sent: Thursday, December 03, 2015 4:43 PM >> To: Smith, Kevin B <kevin.b.smith at intel.com> >> Cc: llvm-dev at lists.llvm.org >> Subject: Re: [llvm-dev] analyzePhysReg question >> >> >> On Dec 3, 2015, at 4:35 PM, Smith, Kevin B via llvm-dev <llvm- >> >> dev at lists.llvm.org> wrote: >> >> >> I am looking at results from analyzePhysReg, and am getting results a little >> >> different than I expected for x86. >> >> >> The call to this is coming from this code in >> >> llvm::MachineBasicBlock::computeRegisterLiveness >> >> 1163 MachineOperandIteratorBase::PhysRegInfo Analysis >> 1164 ConstMIOperands(I).analyzePhysReg(Reg, TRI); >> >> The instruction I being analyzed is: >> %BX<def> = MOV16rm %EDI, 2, %ECX, 0, %noreg; >> >> mem:LD2[%arrayidx98](tbaa=!18) >> >> >> and the Reg being passed in is 21, which is EBX. The result I get back for >> >> is: >> >> >> Analysis: {Clobbers = true, Defines = true, Reads = false, ReadsOverlap >> >> false, >> >> DefinesDead = false, Kills = false} >> >> It seems based on the comment in the definition of PhysRegInfo.Defines, >> >> that Defines should only be true if Reg or a super-register of Reg is >> >> defined. BX is not a super-register of EBX, so it seemed like Defines >> >> should be false here, while Clobbers is correct as true. >> >> I believe the documentation is wrong here. >> EBX is partly defined, so to me Defines == true is conservatively correct >> here. >> My guess, but I haven’t checked the code, is that super-register should be >> understood as any alias of Reg. >> >> Worth checking. >> >> Q. >> >> >> I agree EBX is partly defined, but I think that is what Clobbers is for. >> >> >> I think Clobbers is when there is a RegMask. >> Basically, if that helps, >> Clobber: The register cannot live through. >> Define: (Part of) the register is define by this instruction. >> >> >> Put differently, you can use a register as a definition of an instruction >> when it is clobbered but not defined. >> >> >> The interface for isSuperRegister >> certainly makes a pretty clear distinction between something being a >> superRegister and something being an >> overlapping register. What do you think I should be checking to >understand >> the assumptions/expectations better? >> >> >> The code :). >> >> >> >> >> I wanted to be sure that I wasn't missing something about the interface >> >> definition/expectation. >> >> >> Thanks, >> Kevin Smith >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> > > > >-- >Sanjoy Das >http://playingwithpointers.com >_______________________________________________ >LLVM Developers mailing list >llvm-dev at lists.llvm.org >http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
On Fri, Dec 4, 2015 at 8:55 AM, Smith, Kevin B via llvm-dev < llvm-dev at lists.llvm.org> wrote:> > > >-----Original Message----- > >From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of > >Sanjoy Das via llvm-dev > >Sent: Thursday, December 03, 2015 11:16 PM > >To: Quentin Colombet <qcolombet at apple.com> > >Cc: llvm-dev at lists.llvm.org > >Subject: Re: [llvm-dev] analyzePhysReg question > > > >I think this is related to PR25033: > >https://llvm.org/bugs/show_bug.cgi?id=25033#c9 > > Yes, I agree this is very closely related. The code referenced in that PR > if (Analysis.Kills || Analysis.Clobbers) > // Register killed, so isn't live. > return LQR_Dead; > > is incorrect I think. A clobber isn't a full def, and so cannot be > assumed to be a kill. Some bits from the live-in value may pass through > into the live-out value. > > Looking into the code for anaylzePhysReg, I think the code looks like it > intends for Defines to truly mean Reg or a super-register of Reg is > defined, not any overlapping Reg is defined. > The code looks like this: > > bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(MOReg, Reg); > ... > if (IsRegOrSuperReg) { > PRI.Defines = true; // Reg or a super-register is defined. > if (!MO.isDead()) > AllDefsDead = false; > } > > I think the fundamental bug here is that the operands are swapped when > passed into isSuperRegister. The definition of isSuperRegister is > > /// \brief Returns true if RegB is a super-register of RegA. > bool isSuperRegister(unsigned RegA, unsigned RegB) const; > > so, it looks to me as if in the call to isSuperRegister, the parameters > are swapped, and analyzePhysReg should really be asking whether > the operand Reg (MOReg) is a super register of Reg, and the code should be: > > bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(Reg, MOReg);It would be good to check that this maps correctly onto computeRegisterLiveness: there's a bug in analyzePhysReg and I think other parts of the code base are slightly wrong or will become slightly wrong as well :-( -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20151204/84a3d31e/attachment.html>
Ø It would be good to check that this maps correctly onto computeRegisterLiveness: there's a bug in analyzePhysReg and I think other parts of the code base are slightly wrong or will become slightly wrong as well :-( Yes, I agree. I will also have to look into all other users of analyzePhysReg as well. There are surprisingly few users of either computeRegisterLiveness or analyzePhysReg. The other thing that I am trying to think about would be a way to comprehensively test the correctness of these, both as they stand, and after changes. I am curious, does anyone know how do get this kind of Machine IR to be created from LLVM: xor eax, eax movw ax, mem read eax where the actual intent is for the xor, and the sub-register def in the movw to create form of zero-extension, and then to have full uses of eax? I know this isn't very desirable code for X86, but these kinds of unusual machine IR are what would stress the implementations of these routines. Kevin Smith From: JF Bastien [mailto:jfb at google.com] Sent: Friday, December 04, 2015 1:00 PM To: Smith, Kevin B <kevin.b.smith at intel.com> Cc: Sanjoy Das <sanjoy at playingwithpointers.com>; Quentin Colombet <qcolombet at apple.com>; llvm-dev at lists.llvm.org Subject: Re: [llvm-dev] analyzePhysReg question On Fri, Dec 4, 2015 at 8:55 AM, Smith, Kevin B via llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:>-----Original Message----- >From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org<mailto:llvm-dev-bounces at lists.llvm.org>] On Behalf Of >Sanjoy Das via llvm-dev >Sent: Thursday, December 03, 2015 11:16 PM >To: Quentin Colombet <qcolombet at apple.com<mailto:qcolombet at apple.com>> >Cc: llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org> >Subject: Re: [llvm-dev] analyzePhysReg question > >I think this is related to PR25033: >https://llvm.org/bugs/show_bug.cgi?id=25033#c9Yes, I agree this is very closely related. The code referenced in that PR if (Analysis.Kills || Analysis.Clobbers) // Register killed, so isn't live. return LQR_Dead; is incorrect I think. A clobber isn't a full def, and so cannot be assumed to be a kill. Some bits from the live-in value may pass through into the live-out value. Looking into the code for anaylzePhysReg, I think the code looks like it intends for Defines to truly mean Reg or a super-register of Reg is defined, not any overlapping Reg is defined. The code looks like this: bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(MOReg, Reg); ... if (IsRegOrSuperReg) { PRI.Defines = true; // Reg or a super-register is defined. if (!MO.isDead()) AllDefsDead = false; } I think the fundamental bug here is that the operands are swapped when passed into isSuperRegister. The definition of isSuperRegister is /// \brief Returns true if RegB is a super-register of RegA. bool isSuperRegister(unsigned RegA, unsigned RegB) const; so, it looks to me as if in the call to isSuperRegister, the parameters are swapped, and analyzePhysReg should really be asking whether the operand Reg (MOReg) is a super register of Reg, and the code should be: bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(Reg, MOReg); It would be good to check that this maps correctly onto computeRegisterLiveness: there's a bug in analyzePhysReg and I think other parts of the code base are slightly wrong or will become slightly wrong as well :-( -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20151204/30282f3a/attachment.html>