[llvm] r369191 - [CodeGen] Do the Simple Early Return in block-placement pass to optimize the blocks
Jinsong Ji via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 27 08:08:20 PDT 2019
@skatkov is also reporting crash in D63972#1646303
Contacted @ZhangKang, and revert the commit on behalf of him in
https://reviews.llvm.org/rL370069.
Reproducer is appreciated. Thanks.
Best,
Jinsong Ji (纪金松), PhD.
XL/LLVM on Power Compiler Development
E-mail: jji at us.ibm.com
From: "Mikael Holmén via llvm-commits" <llvm-commits at lists.llvm.org>
To: "fedor.sergeev at azul.com" <fedor.sergeev at azul.com>,
"shkzhang at cn.ibm.com" <shkzhang at cn.ibm.com>
Cc: "llvm-commits at lists.llvm.org" <llvm-commits at lists.llvm.org>
Date: 08/27/2019 01:20 AM
Subject: [EXTERNAL] Re: [llvm] r369191 - [CodeGen] Do the Simple Early
Return in block-placement pass to optimize the blocks
Sent by: "llvm-commits" <llvm-commits-bounces at lists.llvm.org>
On Tue, 2019-08-27 at 01:01 +0300, Fedor Sergeev wrote:
> Hmm... we are seeing problems with X86 backend (and our Java JIT
> frontend) that we managed
> to bisect down to this change.
> I might be able to provide a clean reproducer tomorrow.
>
> Meanwhile, Mikael, was there a bug filed?
> I dont see any followup commits related to this....
>
Hi,
Turned out my problem was specific to my out-of-tree backend where we
run MBP both before and after regalloc.
When we ran it before RA (which isn't done in-tree anywhere as far as I
know) it didn't update PHIs when it changed the CFG and then the
verifier complained.
/Mikael
> regards,
> Fedor.
>
>
> On 8/23/19 4:36 PM, Mikael Holmén via llvm-commits wrote:
> > Hi,
> >
> > We're seeing a problem with this commit for our out-of-tree
> > backend.
> >
> > Before 'Early Branch Probability Basic Block Placement':
> >
> > bb.2.cond.end.thread:
> > ; predecessors: %bb.1
> > successors: %bb.5(0x80000000); %bb.5(100.00%)
> >
> > brr_uncond %bb.5
> >
> > [...]
> >
> > bb.5.land.lhs.true28:
> > ; predecessors: %bb.2, %bb.3
> > successors: %bb.6(0x30000000), %bb.7(0x50000000);
> > %bb.6(37.50%),
> > %bb.7(62.50%)
> >
> > %8:an32_rn_pairs = ...
> > cmp ...
> > brr_cond %bb.7
> > brr_uncond %bb.6
> >
> > [...]
> >
> > bb.7.cond.end38:
> > ; predecessors: %bb.5, %bb.4, %bb.6
> > successors: %bb.8(0x00000001), %bb.9(0x7fffffff); %bb.8(0.00%),
> > %bb.9(100.00%)
> >
> > %2:an32_0_7 = PHI %8:an32_rn_pairs, %bb.5, %11:an32_rn_pairs,
> > %bb.4,
> > %0:an32_rn_pairs, %bb.6
> > cmps ...
> > brr_cond %bb.9
> > brr_uncond %bb.8
> >
> >
> > and after:
> >
> > bb.2.cond.end.thread:
> > ; predecessors: %bb.1, %bb.3
> > successors: %bb.7(0x50000000), %bb.6(0x30000000);
> > %bb.7(62.50%),
> > %bb.6(37.50%)
> >
> > %8:an32_rn_pairs =...
> > cmp ...
> > brr_cond %bb.7
> > brr_uncond %bb.6
> >
> > [...]
> >
> > bb.7.cond.end38:
> > ; predecessors: %bb.4, %bb.6, %bb.2
> > successors: %bb.8(0x00000001), %bb.9(0x7fffffff); %bb.8(0.00%),
> > %bb.9(100.00%)
> >
> > %2:an32_0_7 = PHI %8:an32_rn_pairs, %bb.-1, %11:an32_rn_pairs,
> > %bb.4,
> > %0:an32_rn_pairs, %bb.6
> > cmps ...
> > brr_cond %bb.8
> >
> >
> > So it looks like it's failed to update the PHI operand for %bb.5 in
> > %2
> > properly leaving it now with %bb.-1?
> >
> > Any idea?
> >
> > /Mikael
> >
> > On 2019-08-17 16:37, Kang Zhang via llvm-commits wrote:
> > > Author: zhangkang
> > > Date: Sat Aug 17 07:37:05 2019
> > > New Revision: 369191
> > >
> > > URL:
> > >
https://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye.com_url-3Fk-3D6f40a04c-2D3393b024-2D6f40e0d7-2D8691b328a8b8-2D90715b879d6c2a6b-26q-3D1-26u-3Dhttp-253A-252F-252Fllvm.org-252Fviewvc-252Fllvm-2Dproject-253Frev-253D369191-2526view-253Drev&d=DwIGaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=DvnnfavFQBGT2CDyHzTr_Q&m=qowb0NV4OShnAsWRVoKI4HOjJWkjhypcfrb76YZ8HPI&s=PuMcx59HrAuyvEtIKdGhER96DZt18-HfWBT_jWQ-QvU&e=
> > > Log:
> > > [CodeGen] Do the Simple Early Return in block-placement pass to
> > > optimize the blocks
> > >
> > > Summary:
> > >
> > > Fix a bug of preducessors.
> > >
> > > In `block-placement` pass, it will create some patterns for
> > > unconditional we can do the simple early retrun.
> > > But the `early-ret` pass is before `block-placement`, we don't
> > > want to run it again.
> > > This patch is to do the simple early return to optimize the
> > > blocks at the last of `block-placement`.
> > >
> > > Reviewed By: efriedma
> > >
> > > Differential Revision:
> > >
https://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye.com_url-3Fk-3Daf91b373-2Df342a31b-2Daf91f3e8-2D8691b328a8b8-2Da356f6b65b5e7513-26q-3D1-26u-3Dhttps-253A-252F-252Freviews.llvm.org-252FD63972&d=DwIGaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=DvnnfavFQBGT2CDyHzTr_Q&m=qowb0NV4OShnAsWRVoKI4HOjJWkjhypcfrb76YZ8HPI&s=AhiL4CVTRD60hnx7f9dW3Rif6zzQps9qTEfaZclQ8zA&e=
> > >
> > > Modified:
> > > llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
> > > llvm/trunk/test/CodeGen/PowerPC/block-placement.mir
> > >
> > > Modified: llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp
> > > URL:
> > >
https://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye.com_url-3Fk-3D44a01ed0-2D18730eb8-2D44a05e4b-2D8691b328a8b8-2D590610f76548b1fa-26q-3D1-26u-3Dhttp-253A-252F-252Fllvm.org-252Fviewvc-252Fllvm-2Dproject-252Fllvm-252Ftrunk-252Flib-252FCodeGen-252FMachineBlockPlacement.cpp-253Frev-253D369191-2526r1-253D369190-2526r2-253D369191-2526view-253Ddiff&d=DwIGaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=DvnnfavFQBGT2CDyHzTr_Q&m=qowb0NV4OShnAsWRVoKI4HOjJWkjhypcfrb76YZ8HPI&s=0x7bHzqYamkVyfKPD-W3wPYrnNDuqyGO0Z8agOX_UDs&e=
> > > =================================================================
> > > =============
> > > --- llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp (original)
> > > +++ llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp Sat Aug 17
> > > 07:37:05 2019
> > > @@ -2711,6 +2711,7 @@ void MachineBlockPlacement::optimizeBran
> > > // cannot because all branches may not be analyzable.
> > > // E.g., the target may be able to remove an unconditional
> > > branch to
> > > // a fallthrough when it occurs after predicated
> > > terminators.
> > > + SmallVector<MachineBasicBlock*, 4> EmptyBB;
> > > for (MachineBasicBlock *ChainBB : FunctionChain) {
> > > Cond.clear();
> > > MachineBasicBlock *TBB = nullptr, *FBB = nullptr; // For
> > > AnalyzeBranch.
> > > @@ -2730,9 +2731,45 @@ void MachineBlockPlacement::optimizeBran
> > > TII->removeBranch(*ChainBB);
> > > TII->insertBranch(*ChainBB, FBB, TBB, Cond, dl);
> > > ChainBB->updateTerminator();
> > > + } else if (Cond.empty() && TBB && ChainBB != TBB && !TBB-
> > > >empty() &&
> > > + !TBB->canFallThrough()) {
> > > + // When ChainBB is unconditional branch to the TBB, and
> > > TBB has no
> > > + // fallthrough predecessor and fallthrough successor,
> > > try to merge
> > > + // ChainBB and TBB. This is legal under the one of
> > > following conditions:
> > > + // 1. ChainBB is empty except for an unconditional
> > > branch.
> > > + // 2. TBB has only one predecessor.
> > > + MachineFunction::iterator I(TBB);
> > > + if (((TBB == &*F->begin()) || !std::prev(I)-
> > > >canFallThrough()) &&
> > > + (TailDup.isSimpleBB(ChainBB) || (TBB->pred_size()
> > > == 1))) {
> > > + TII->removeBranch(*ChainBB);
> > > + ChainBB->removeSuccessor(TBB);
> > > +
> > > + // Update the CFG.
> > > + while (!TBB->pred_empty()) {
> > > + MachineBasicBlock *Pred = *(TBB->pred_end() - 1);
> > > + Pred->ReplaceUsesOfBlockWith(TBB, ChainBB);
> > > + }
> > > +
> > > + while (!TBB->succ_empty()) {
> > > + MachineBasicBlock *Succ = *(TBB->succ_end() - 1);
> > > + ChainBB->addSuccessor(Succ, MBPI-
> > > >getEdgeProbability(TBB, Succ));
> > > + TBB->removeSuccessor(Succ);
> > > + }
> > > +
> > > + // Move all the instructions of TBB to ChainBB.
> > > + ChainBB->splice(ChainBB->end(), TBB, TBB->begin(),
> > > TBB->end());
> > > + EmptyBB.push_back(TBB);
> > > + }
> > > }
> > > }
> > > }
> > > +
> > > + for (auto BB: EmptyBB) {
> > > + MLI->removeBlock(BB);
> > > + FunctionChain.remove(BB);
> > > + BlockToChain.erase(BB);
> > > + F->erase(BB);
> > > + }
> > > }
> > >
> > > void MachineBlockPlacement::alignBlocks() {
> > > @@ -3052,6 +3089,9 @@ bool MachineBlockPlacement::runOnMachine
> > > }
> > > }
> > >
> > > + // optimizeBranches() may change the blocks, but we haven't
> > > updated the
> > > + // post-dominator tree. Because the post-dominator tree won't
> > > be used after
> > > + // this function and this pass don't preserve the post-
> > > dominator tree.
> > > optimizeBranches();
> > > alignBlocks();
> > >
> > >
> > > Modified: llvm/trunk/test/CodeGen/PowerPC/block-placement.mir
> > > URL:
> > >
https://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye.com_url-3Fk-3Dc2447a57-2D9e976a3f-2Dc2443acc-2D8691b328a8b8-2D7cd3fcb165ec6683-26q-3D1-26u-3Dhttp-253A-252F-252Fllvm.org-252Fviewvc-252Fllvm-2Dproject-252Fllvm-252Ftrunk-252Ftest-252FCodeGen-252FPowerPC-252Fblock-2Dplacement.mir-253Frev-253D369191-2526r1-253D369190-2526r2-253D369191-2526view-253Ddiff&d=DwIGaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=DvnnfavFQBGT2CDyHzTr_Q&m=qowb0NV4OShnAsWRVoKI4HOjJWkjhypcfrb76YZ8HPI&s=hTHVxvGRUHQKNJ-em3lOWtyAt7A6J_eo6I39BKyVR4w&e=
> > > =================================================================
> > > =============
> > > --- llvm/trunk/test/CodeGen/PowerPC/block-placement.mir
> > > (original)
> > > +++ llvm/trunk/test/CodeGen/PowerPC/block-placement.mir Sat Aug
> > > 17 07:37:05 2019
> > > @@ -209,14 +209,10 @@ body: |
> > > BLR8 implicit $lr8, implicit $rm, implicit killed $x3
> > >
> > > ; CHECK: bb.5.if.else.i:
> > > - ; CHECK: successors: %bb.11(0x80000000)
> > > - ; CHECK: B %bb.11
> > > + ; CHECK-NEXT: renamable $x3 = LI8 1
> > > + ; CHECK-NEXT: BLR8 implicit $lr8, implicit $rm, implicit
> > > killed $x3
> > >
> > > ; CHECK: bb.8.while.body.i (align 4):
> > > - ; CHECK: successors: %bb.11(0x04000000),
> > > %bb.9(0x7c000000)
> > > - ; CHECK: BCC 76, killed renamable $cr0, %bb.11
> > > -
> > > - ; CHECK: bb.11:
> > > - ; CHECK: renamable $x3 = LI8 1
> > > - ; CHECK-NEXT: BLR8 implicit $lr8, implicit $rm, implicit
> > > killed $x3
> > > + ; CHECK: successors: %bb.5(0x04000000),
> > > %bb.9(0x7c000000)
> > > + ; CHECK: BCC 76, killed renamable $cr0, %bb.5
> > > ...
> > >
> > >
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at lists.llvm.org
> > >
https://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye.com_url-3Fk-3Db7a41100-2Deb770168-2Db7a4519b-2D8691b328a8b8-2D00280ce3331de496-26q-3D1-26u-3Dhttps-253A-252F-252Flists.llvm.org-252Fcgi-2Dbin-252Fmailman-252Flistinfo-252Fllvm-2Dcommits&d=DwIGaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=DvnnfavFQBGT2CDyHzTr_Q&m=qowb0NV4OShnAsWRVoKI4HOjJWkjhypcfrb76YZ8HPI&s=-JQz9AeaSQFe1L3hymFGVB7C56k29GHv_7cTjQoG2R4&e=
> > >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> >
https://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye.com_url-3Fk-3D5d94a315-2D011e68e1-2D5d94e38e-2D86cd58c48020-2Da1db560ed36cea73-26q-3D1-26u-3Dhttps-253A-252F-252Flists.llvm.org-252Fcgi-2Dbin-252Fmailman-252Flistinfo-252Fllvm-2Dcommits&d=DwIGaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=DvnnfavFQBGT2CDyHzTr_Q&m=qowb0NV4OShnAsWRVoKI4HOjJWkjhypcfrb76YZ8HPI&s=3yYxXn-1lrZghT9lmce-67Odq6kc6V5CpHscUqz22pI&e=
>
>
_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=DwIGaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=DvnnfavFQBGT2CDyHzTr_Q&m=qowb0NV4OShnAsWRVoKI4HOjJWkjhypcfrb76YZ8HPI&s=oXhI96J61xQ1SCEuYXQAX6WNMUrQG0OaPrAjGNoHczg&e=
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190827/cf415ad3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190827/cf415ad3/attachment.gif>
More information about the llvm-commits
mailing list