<div class="socmaildefaultfont" dir="ltr" style="font-family:Arial, Helvetica, sans-serif;font-size:10.5pt" ><div dir="ltr" >Hi all,</div>
<div dir="ltr" > </div>
<div dir="ltr" >Does anyone have any objections to me committing this patch back with it turned off by default?</div>
<div dir="ltr" > </div>
<div dir="ltr" ><div class="socmaildefaultfont" dir="ltr" style="font-family:Arial, Helvetica, sans-serif;font-size:10.5pt" ><div class="socmaildefaultfont" dir="ltr" style="font-family:Arial, Helvetica, sans-serif;font-size:10.5pt" ><div class="socmaildefaultfont" dir="ltr" style="font-family:Arial, Helvetica, sans-serif;font-size:10.5pt" ><div dir="ltr" ><br>Regards,<br>Lei Huang</div>
<div dir="ltr" > </div>
<div dir="ltr" > </div>
<div dir="ltr" >LLVM Development on POWER</div>
<div dir="ltr" >Internal mail: C2/YGK/8200/MKM<br>Phone: (905) 413-4419<br>TieLine: 969-4419<br>E-mail: lei@ca.ibm.com</div></div></div></div></div>
<div dir="ltr" > </div>
<div dir="ltr" > </div>
<blockquote data-history-content-modified="1" data-history-expanded="1" dir="ltr" style="border-left:solid #aaaaaa 2px; margin-left:5px; padding-left:5px; direction:ltr; margin-right:0px" >----- Original message -----<br>From: Eric Christopher <echristo@gmail.com><br>To: reviews+D28249+public+34df2467c7dab1c5@reviews.llvm.org, lei@ca.ibm.com, carrot@google.com, hfinkel@anl.gov, nemanja.i.ibm@gmail.com<br>Cc: kyle+llvm@iteratee.net, matze@braunis.de, hiraditya@msn.com, craig.topper@gmail.com, syzaara@ca.ibm.com, sfertile@ca.ibm.com, jtony@ca.ibm.com, amara.emerson@arm.com, joker.eph@gmail.com, mgorny@gentoo.org, llvm-commits@lists.llvm.org, Daniel Jasper <djasper@google.com><br>Subject: Re: [PATCH] D28249: Improve scheduling with branch coalescing<br>Date: Thu, Aug 31, 2017 2:34 AM<br>
<div dir="ltr" >Hi All,
<div> </div>
<div>Right now since we have a failing testcase internally would anyone mind either:</div>
<div> </div>
<div>a) reverting the patch,</div>
<div>b) turning off the pass temporarily</div>
<div> </div>
<div>while we fix the issues or work up a more public testcase?</div>
<div> </div>
<div> </div>
<div><div dir="ltr" >On Wed, Aug 30, 2017 at 4:33 PM Hal Finkel via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank" >reviews@reviews.llvm.org</a>> wrote:</div>
<blockquote 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://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=" rel="noreferrer" target="_blank" >https://reviews.llvm.org/D28249</a><br><br><br> </blockquote></div></div></blockquote>
<div dir="ltr" > </div></div><BR>