<div dir="ltr">Hi All,<div><br></div><div>Right now since we have a failing testcase internally would anyone mind either:</div><div><br></div><div>a) reverting the patch,</div><div>b) turning off the pass temporarily</div><div><br></div><div>while we fix the issues or work up a more public testcase?</div><div><br></div><div><br></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 30, 2017 at 4:33 PM Hal Finkel via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">hfinkel added inline comments.<br>
<br>
<br>
================<br>
Comment at: llvm/trunk/lib/CodeGen/BranchCoalescing.cpp:340<br>
+      MachineInstr *Op2Def = MRI->getVRegDef(Op2.getReg());<br>
+      if (TII->produceSameValue(*Op1Def, *Op2Def, MRI)) {<br>
+        DEBUG(dbgs() << "Op1Def: " << *Op1Def << " and " << *Op2Def<br>
----------------<br>
iteratee wrote:<br>
> This line isn't correct. I have a case where instructions that "produce the same value" are different. The relevant sequence is:<br>
> ```<br>
> BB#16:<br>
> ...<br>
>         BCTRL8_LDinto_toc<br>
> ...<br>
>         %vreg140<def> = COPY %CR0GT; CRBITRC:%vreg140<br>
>         %vreg141<def> = LXSDX %vreg138, %vreg129, %RM<imp-use>; mem:LD8[%134](dereferenceable) F8RC:%vreg141 G8RC_and_G8RC_NOX0:%vreg138 G8RC:%vreg129<br>
>         %vreg142<def> = XXLXORdpz; F8RC:%vreg142<br>
>         BC %vreg140, <BB#73>; CRBITRC:%vreg140<br>
><br>
> BB#72: derived from LLVM BB %114<br>
>     Predecessors according to CFG: BB#16<br>
>     Successors according to CFG: BB#73(?%)<br>
><br>
> BB#73: derived from LLVM BB %114<br>
>     Predecessors according to CFG: BB#16 BB#72<br>
>         %vreg143<def> = PHI %vreg142, <BB#72>, %vreg141, <BB#16>; F8RC:%vreg143,%vreg142,%vreg141<br>
> ...<br>
>         BCTRL8_LDinto_toc<br>
> ...<br>
>         %vreg149<def> = COPY %CR0GT; CRBITRC:%vreg149<br>
>         %vreg150<def> = LXSDX %vreg138, %vreg129, %RM<imp-use>; mem:LD8[%134](dereferenceable) F8RC:%vreg150 G8RC_and_G8RC_NOX0:%vreg138 G8RC:%vreg129<br>
>         BC %vreg149, <BB#75>; CRBITRC:%vreg149<br>
>     Successors according to CFG: BB#74(?%) BB#75(?%)<br>
><br>
> BB#74: derived from LLVM BB %114<br>
>     Predecessors according to CFG: BB#73<br>
>     Successors according to CFG: BB#75(?%)<br>
><br>
> BB#75: derived from LLVM BB %114<br>
>     Predecessors according to CFG: BB#73 BB#74<br>
>         %vreg151<def> = PHI %vreg142, <BB#74>, %vreg150, <BB#73>; F8RC:%vreg151,%vreg142,%vreg150<br>
> ```<br>
><br>
><br>
> The debug output produces:<br>
> ```<br>
> Valid Candidate<br>
> Op1: 1024<br>
> Op2: 1024<br>
> Op1 and Op2 are identical!<br>
> Op1: %vreg140<br>
> Op2: %vreg149<br>
> Op1Def: %vreg140<def> = COPY %CR0GT; CRBITRC:%vreg140<br>
>  and %vreg149<def> = COPY %CR0GT; CRBITRC:%vreg149<br>
>  produce the same value!<br>
> ```<br>
><br>
> While it would be safe to CSE those crmoves, what definitely cannot occur is to assume that the value of CR0GT has not changed between the 2 instructions.<br>
Indeed; I think that we need to filter out instructions with physical-register uses (even if identical). We might be able to be a little smarter about it in a similar way to MachineLICM (the other uses of this function), which does this:<br>
<br>
    // Don't hoist an instruction that uses or defines a physical register.<br>
    if (TargetRegisterInfo::isPhysicalRegister(Reg)) {<br>
      if (MO.isUse()) {<br>
        // If the physreg has no defs anywhere, it's just an ambient register<br>
        // and we can freely move its uses. Alternatively, if it's allocatable,<br>
        // it could get allocated to something with a def during allocation.<br>
        // However, if the physreg is known to always be caller saved/restored<br>
        // then this use is safe to hoist.<br>
        if (!MRI->isConstantPhysReg(Reg) &&<br>
            !(TRI->isCallerPreservedPhysReg(Reg, *I.getParent()->getParent())))<br>
            return false;<br>
        // Otherwise it's safe to move.<br>
        continue;<br>
      } else if (!MO.isDead()) {<br>
        // A def that isn't dead. We can't move it.<br>
        return false;<br>
      ...<br>
<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D28249" rel="noreferrer" target="_blank">https://reviews.llvm.org/D28249</a><br>
<br>
<br>
<br>
</blockquote></div></div>