<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Dec 4, 2015 at 8:55 AM, Smith, Kevin B via llvm-dev <span dir="ltr"><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><br>
<br>
>-----Original Message-----<br>
>From: llvm-dev [mailto:<a href="mailto:llvm-dev-bounces@lists.llvm.org">llvm-dev-bounces@lists.llvm.org</a>] On Behalf Of<br>
>Sanjoy Das via llvm-dev<br>
>Sent: Thursday, December 03, 2015 11:16 PM<br>
>To: Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>><br>
>Cc: <a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a><br>
>Subject: Re: [llvm-dev] analyzePhysReg question<br>
><br>
</span><span class="">>I think this is related to PR25033:<br>
><a href="https://llvm.org/bugs/show_bug.cgi?id=25033#c9" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=25033#c9</a><br>
<br>
</span>Yes, I agree this is very closely related.  The code referenced in that PR<br>
if (Analysis.Kills || Analysis.Clobbers)<br>
        // Register killed, so isn't live.<br>
        return LQR_Dead;<br>
<br>
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.<br>
<br>
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.<br>
The code looks like this:<br>
<br>
bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(MOReg, Reg);<br>
...<br>
if (IsRegOrSuperReg) {<br>
  PRI.Defines = true;     // Reg or a super-register is defined.<br>
  if (!MO.isDead())<br>
    AllDefsDead = false;<br>
}<br>
<br>
I think the fundamental bug here is that the operands are swapped when passed into isSuperRegister.  The definition of isSuperRegister is<br>
<br>
/// \brief Returns true if RegB is a super-register of RegA.<br>
bool isSuperRegister(unsigned RegA, unsigned RegB) const;<br>
<br>
so, it looks to me as if in the call to isSuperRegister, the parameters are swapped, and analyzePhysReg should really be asking whether<br>
the operand Reg (MOReg) is a super register of Reg, and the code should be:<br>
<br>
bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(Reg, MOReg);</blockquote><div><br></div><div>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 :-(</div></div></div></div>