[llvm-dev] analyzePhysReg question

[Smith, Kevin B via llvm-dev]

Ø  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#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/30282f3a/attachment.html>