[llvm-dev] analyzePhysReg question

[Smith, Kevin B via llvm-dev]

>-----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);

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