analyzePhysReg question

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 wanted to be sure that I wasn't missing something about the interface definition/expectation.

Thanks,
Kevin Smith

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.

From: Quentin Colombet [mailto:qcolombet@apple.com]
Sent: Thursday, December 03, 2015 4:43 PM
To: Smith, Kevin B <kevin.b.smith@intel.com>
Cc: llvm-dev@lists.llvm.org
Subject: Re: [llvm-dev] analyzePhysReg question

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. 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?

From: Quentin Colombet [mailto:qcolombet@apple.com]
Sent: Thursday, December 03, 2015 4:43 PM
To: Smith, Kevin B <kevin.b.smith@intel.com>
Cc: llvm-dev@lists.llvm.org
Subject: Re: [llvm-dev] analyzePhysReg question

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 = MOV16rm %EDI, 2, %ECX, 0, %noreg;

mem:LD2%arrayidx98

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.

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 :).

From: Quentin Colombet [mailto:qcolombet@apple.com]
Sent: Thursday, December 03, 2015 4:43 PM
To: Smith, Kevin B <kevin.b.smith@intel.com>
Cc: llvm-dev@lists.llvm.org
Subject: Re: [llvm-dev] analyzePhysReg question

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 = MOV16rm %EDI, 2, %ECX, 0, %noreg;

mem:LD2%arrayidx98

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.

I think this is related to PR25033:
https://llvm.org/bugs/show_bug.cgi?id=25033#c9

-- Sanjoy

From: llvm-dev [mailto:llvm-dev-bounces@lists.llvm.org] On Behalf Of
Sanjoy Das via llvm-dev
Sent: Thursday, December 03, 2015 11:16 PM
To: Quentin Colombet <qcolombet@apple.com>
Cc: llvm-dev@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 :frowning:

Ø 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 :frowning:

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

Ø 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 :frowning:

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?

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