[PATCH] D28249: Improve scheduling with branch coalescing

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: llvm/trunk/lib/CodeGen/BranchCoalescing.cpp:340
+      MachineInstr *Op2Def = MRI->getVRegDef(Op2.getReg());
+      if (TII->produceSameValue(*Op1Def, *Op2Def, MRI)) {
+        DEBUG(dbgs() << "Op1Def: " << *Op1Def << " and " << *Op2Def
----------------
iteratee wrote:
> This line isn't correct. I have a case where instructions that "produce the same value" are different. The relevant sequence is:
> ```
> BB#16: 
> ...
>         BCTRL8_LDinto_toc
> ...
>         %vreg140<def> = COPY %CR0GT; CRBITRC:%vreg140
>         %vreg141<def> = LXSDX %vreg138, %vreg129, %RM<imp-use>; mem:LD8[%134](dereferenceable) F8RC:%vreg141 G8RC_and_G8RC_NOX0:%vreg138 G8RC:%vreg129
>         %vreg142<def> = XXLXORdpz; F8RC:%vreg142
>         BC %vreg140, <BB#73>; CRBITRC:%vreg140
> 
> BB#72: derived from LLVM BB %114
>     Predecessors according to CFG: BB#16
>     Successors according to CFG: BB#73(?%)
> 
> BB#73: derived from LLVM BB %114
>     Predecessors according to CFG: BB#16 BB#72
>         %vreg143<def> = PHI %vreg142, <BB#72>, %vreg141, <BB#16>; F8RC:%vreg143,%vreg142,%vreg141
> ...
>         BCTRL8_LDinto_toc
> ...
>         %vreg149<def> = COPY %CR0GT; CRBITRC:%vreg149
>         %vreg150<def> = LXSDX %vreg138, %vreg129, %RM<imp-use>; mem:LD8[%134](dereferenceable) F8RC:%vreg150 G8RC_and_G8RC_NOX0:%vreg138 G8RC:%vreg129
>         BC %vreg149, <BB#75>; CRBITRC:%vreg149
>     Successors according to CFG: BB#74(?%) BB#75(?%)
> 
> BB#74: derived from LLVM BB %114
>     Predecessors according to CFG: BB#73
>     Successors according to CFG: BB#75(?%)
> 
> BB#75: derived from LLVM BB %114
>     Predecessors according to CFG: BB#73 BB#74
>         %vreg151<def> = PHI %vreg142, <BB#74>, %vreg150, <BB#73>; F8RC:%vreg151,%vreg142,%vreg150
> ```
> 
> 
> The debug output produces:
> ```
> Valid Candidate
> Op1: 1024
> Op2: 1024
> Op1 and Op2 are identical!
> Op1: %vreg140
> Op2: %vreg149
> Op1Def: %vreg140<def> = COPY %CR0GT; CRBITRC:%vreg140
>  and %vreg149<def> = COPY %CR0GT; CRBITRC:%vreg149
>  produce the same value!
> ```
> 
> 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.
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:

    // Don't hoist an instruction that uses or defines a physical register.
    if (TargetRegisterInfo::isPhysicalRegister(Reg)) {
      if (MO.isUse()) {
        // If the physreg has no defs anywhere, it's just an ambient register
        // and we can freely move its uses. Alternatively, if it's allocatable,
        // it could get allocated to something with a def during allocation.
        // However, if the physreg is known to always be caller saved/restored
        // then this use is safe to hoist.
        if (!MRI->isConstantPhysReg(Reg) &&
            !(TRI->isCallerPreservedPhysReg(Reg, *I.getParent()->getParent())))
            return false;
        // Otherwise it's safe to move.
        continue;
      } else if (!MO.isDead()) {
        // A def that isn't dead. We can't move it.
        return false;
      ...



Repository:
  rL LLVM

https://reviews.llvm.org/D28249