[PATCH] D28249: Improve scheduling with branch coalescing

Eric Christopher via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 30 22:35:14 PDT 2017


Hi All,

Right now since we have a failing testcase internally would anyone mind
either:

a) reverting the patch,
b) turning off the pass temporarily

while we fix the issues or work up a more public testcase?



On Wed, Aug 30, 2017 at 4:33 PM Hal Finkel via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170831/7bd627ac/attachment.html>


More information about the llvm-commits mailing list