[PATCH] D28249: Improve scheduling with branch coalescing

Hal Finkel via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 31 11:21:11 PDT 2017


On 08/31/2017 01:09 PM, Lei Huang wrote:
> Hi all,
> Does anyone have any objections to me committing this patch back with 
> it turned off by default?

I think that's fine. Then you can post a patch to fix this bug.

  -Hal

>
> Regards,
> Lei Huang
> LLVM Development on POWER
> Internal mail: C2/YGK/8200/MKM
> Phone: (905) 413-4419
> TieLine: 969-4419
> E-mail: lei at ca.ibm.com
>
>     ----- Original message -----
>     From: Eric Christopher <echristo at gmail.com>
>     To: reviews+D28249+public+34df2467c7dab1c5 at reviews.llvm.org,
>     lei at ca.ibm.com, carrot at google.com, hfinkel at anl.gov,
>     nemanja.i.ibm at gmail.com
>     Cc: kyle+llvm at iteratee.net, matze at braunis.de, hiraditya at msn.com,
>     craig.topper at gmail.com, syzaara at ca.ibm.com, sfertile at ca.ibm.com,
>     jtony at ca.ibm.com, amara.emerson at arm.com, joker.eph at gmail.com,
>     mgorny at gentoo.org, llvm-commits at lists.llvm.org, Daniel Jasper
>     <djasper at google.com>
>     Subject: Re: [PATCH] D28249: Improve scheduling with branch coalescing
>     Date: Thu, Aug 31, 2017 2:34 AM
>     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 <mailto: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
>         <https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D28249&d=DwMFaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=wcuzqlo__GgcNlt1_YAUPg&m=tHTjrXS2HNkfTEF9LX-FDD0fduLEs2MiEMJPQ2M53pQ&s=pF0fBNDL_Koy8xlMpGdnOFqnTdHitMwgjGYkLJ6tY_I&e=>
>
>
>

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170831/a77023e6/attachment.html>


More information about the llvm-commits mailing list