Ø 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>
On Fri, Dec 4, 2015 at 2:22 PM, Smith, Kevin B <kevin.b.smith at intel.com> wrote:> Ø 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. >Great, thanks! I'd be happy to review this, I just don't have much time to tackle the problem myself. This code should also be fixed at the same time: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20140317/209514.html 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. >I'm not sure, sorry. You could play tricks with asm and clobbers? -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20151204/7a8e1d58/attachment.html>
I noticed a similar problem with ah when asking about eax for x86. An interesting thing is that for my purposes (and why I am looking into this), I kind of want to ignore "certain" sub registers. The actual problem I am trying to solve is to be at an instruction like: ax = movw mem or al = movb mem and be able to ask whether any part of rax is live other than the part that is clearly being defined by the move. computeRegisterLiveness seems the right place to do that, but I don't think the exact same interface currently in use cannot solve what I want and solve http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20140317/209514.html my expectation is that computeRegisterLiveness should return LQR_OverlappingLive whenever a sub-reg or super-reg is live, but the exact register that was passed in, is not live. Does that sound like the right to you folks? Kevin From: JF Bastien [mailto:jfb at google.com] Sent: Friday, December 04, 2015 2:33 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 2:22 PM, Smith, Kevin B <kevin.b.smith at intel.com<mailto:kevin.b.smith at intel.com>> wrote:> 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. Great, thanks! I'd be happy to review this, I just don't have much time to tackle the problem myself. This code should also be fixed at the same time: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20140317/209514.html 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. I'm not sure, sorry. You could play tricks with asm and clobbers? -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20151204/4b98d4c2/attachment.html>