[llvm] r324572 - [SimplifyCFG] Re-apply Relax restriction for folding unconditional branches
Serguei Katkov via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 13 21:01:26 PST 2018
Hi David,
According to what I changed in the patch...
In reality we are not remove a pre-header. The patch actually added the check for the number of predecessors (should be equal to 1 to allow elimination of this BB). So if pre-header has only one predecessor it will be eliminated and actually its predecessor will be the new pre-header.
The difference I see in your cases (my guess):
Before the patch - the pre-header might be empty (my guess) and had only one predecessor.
After the patch - the pre-header might be non-empty and potentially might have several predecessors.
It is not clear actually what optimization or backend depends on these conditions... It is really strange.
And I agree that I do not see an evident way how to detect the BB is a pre-header but not a loop member...
I would suggest first to focus on analysis what is wrong with changing pre-header to its predecessor. It is possible that the problem is not here or may be narrow down the more strict condition to prevent this change and it will not require the knowledge about whether this is pre-header or not.
Does it make sense for you?
Thank you,
Serguei.
> -----Original Message-----
> From: David Green [mailto:David.Green at arm.com]
> Sent: Wednesday, February 14, 2018 1:07 AM
> To: Serguei Katkov <serguei.katkov at azul.com>
> Cc: nd <nd at arm.com>; llvm-commits <llvm-commits at lists.llvm.org>
> Subject: Re: [llvm] r324572 - [SimplifyCFG] Re-apply Relax restriction for
> folding unconditional branches
>
> Hello
>
> Yes, I agree it's bit odd. I think the preheaders are recreated before they are
> needed, so it may just be code-ordering that's leading to the regressions.
> That's what I've seen so far at least, I've not managed to tell why the
> ordering leads to the differences yet. I'll keep looking.
>
> But.. as far as I can tell the intent of this code is to preserve loop simple form,
> and if we are removing the preheader then we are not doing that. Any ideas
> how to sensibly prevent this from acting on preheaders? If we had loop info
> we could check if the BB is in the loop, without that I'm not sure how to tell
> the two types of blocks apart correctly. I don't think we have a domtree to
> check that way either.
>
> Dave
>
>
> From: Serguei Katkov <serguei.katkov at azul.com>
> Sent: 13 February 2018 17:11
> To: Serguei Katkov; David Green
> Cc: nd; llvm-commits
> Subject: RE: [llvm] r324572 - [SimplifyCFG] Re-apply Relax restriction for
> folding unconditional branches
>
> Just to clarify (I'm not sure I was clear here :)).
>
> 1) The motivation was to remove BBs in a loop.
> 2) Removing of empty pre-header is a kind of side-effect.
> 3) It is interesting why removing of pre-headers results in perfromance
> regressions.
>
> Thank you,
> Serguei.
>
> > -----Original Message-----
> > From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On
> > Behalf Of Serguei Katkov via llvm-commits
> > Sent: Wednesday, February 14, 2018 12:08 AM
> > To: David Green <David.Green at arm.com>; llvm-commits <llvm-
> > commits at lists.llvm.org>
> > Cc: nd <nd at arm.com>
> > Subject: RE: [llvm] r324572 - [SimplifyCFG] Re-apply Relax restriction
> > for folding unconditional branches
> >
> > Hello David,
> >
> > the main idea was removing empty BB in a loop coming to header (latch)
> > but not a removing of pre-header actually.
> >
> > However it is not clear why removing an empty pre-header comes to
> > regressions. Jut interesting to know about the reason.
> >
> > Thank you,
> > Serguei.
> >
> > > -----Original Message-----
> > > From: David Green [mailto:David.Green at arm.com]
> > > Sent: Tuesday, February 13, 2018 11:40 PM
> > > To: Serguei Katkov <serguei.katkov at azul.com>; llvm-commits <llvm-
> > > commits at lists.llvm.org>
> > > Cc: nd <nd at arm.com>
> > > Subject: Re: [llvm] r324572 - [SimplifyCFG] Re-apply Relax
> > > restriction for folding unconditional branches
> > >
> > > Hello
> > >
> > > First up, sorry for the lateness of this response.
> > >
> > > Secondly, I am seeing a fair number of small to medium size
> > > performance regressions from this patch, without many improvements
> > > (on ARM). Is the idea that this should be removing preheaders? Or
> > > just empty
> > blocks in loops?
> > >
> > > The only regressions I have analysed so far have turned into "code
> > > is in a different order, the backend does something slightly
> > > different as a
> > result".
> > > And many regressions only appear to show up when LTO is enabled.
> > >
> > > So I don't have any sensible reproducers yet, but any thoughts about
> > > it removing preheaders? It doesn't seem like that was the intent.
> > > Dave
> > >
> > >
> > >
> > > From: llvm-commits <llvm-commits-bounces at lists.llvm.org> on behalf
> > > of Serguei Katkov via llvm-commits <llvm-commits at lists.llvm.org>
> > > Sent: 08 February 2018 07:16
> > > To: llvm-commits at lists.llvm.org
> > > Subject: [llvm] r324572 - [SimplifyCFG] Re-apply Relax restriction
> > > for folding unconditional branches
> > >
> > > Author: skatkov
> > > Date: Wed Feb 7 23:16:29 2018
> > > New Revision: 324572
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=324572&view=rev
> > > Log:
> > > [SimplifyCFG] Re-apply Relax restriction for folding unconditional
> > > branches
> > >
> > > The commit rL308422 introduces a restriction for folding
> > > unconditional branches. Specifically if empty block with
> > > unconditional branch leads to header of the loop then elimination of this
> basic block is prohibited.
> > > However it seems this condition is redundantly strict.
> > > If elimination of this basic block does not introduce more back
> > > edges then we can eliminate this block.
> > >
> > > The patch implements this relax of restriction.
> > >
> > > The test profile/Linux/counter_promo_nest.c in compiler-rt project
> > > is updated to meet this change.
> > >
> > > Reviewers: efriedma, mcrosier, pacxx, hsung, davidxl Reviewed By:
> > > pacxx
> > > Subscribers: llvm-commits
> > > Differential Revision: https://reviews.llvm.org/D42691
> > >
> > > Added:
> > > llvm/trunk/test/Transforms/SimplifyCFG/UncondBranchToHeader.ll
> > > Modified:
> > > llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
> > > llvm/trunk/test/Transforms/LoopUnroll/peel-loop.ll
> > > llvm/trunk/test/Transforms/LoopUnswitch/2015-06-17-Metadata.ll
> > > llvm/trunk/test/Transforms/LoopUnswitch/infinite-loop.ll
> > >
> > > Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
> > > URL: http://llvm.org/viewvc/llvm-
> > > project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=324572&r
> > > 1=
> > > 32
> > > 4571&r2=324572&view=diff
> > >
> >
> ==========================================================
> > > ====================
> > > --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
> > > +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Wed Feb 7
> > > +++ 23:16:29
> > > +++ 2018
> > > @@ -5733,9 +5733,12 @@ bool SimplifyCFGOpt::SimplifyUncondBranc
> > > // header. (This is for early invocations before loop simplify and
> > > // vectorization to keep canonical loop forms for nested loops.
> > > These
> > blocks
> > > // can be eliminated when the pass is invoked later in the
> > > back-end.)
> > > + // Note that if BB has only one predecessor then we do not
> > > + introduce new // backedge, so we can eliminate BB.
> > > bool NeedCanonicalLoop =
> > > Options.NeedCanonicalLoop &&
> > > - (LoopHeaders && (LoopHeaders->count(BB) || LoopHeaders-
> > > >count(Succ)));
> > > + (LoopHeaders && std::distance(pred_begin(BB), pred_end(BB)) >
> > > + 1
> > &&
> > > + (LoopHeaders->count(BB) || LoopHeaders->count(Succ)));
> > > BasicBlock::iterator I = BB->getFirstNonPHIOrDbg()->getIterator();
> > > if (I->isTerminator() && BB != &BB->getParent()->getEntryBlock() &&
> > > !NeedCanonicalLoop &&
> > > TryToSimplifyUncondBranchFromEmptyBlock(BB))
> > >
> > > Modified: llvm/trunk/test/Transforms/LoopUnroll/peel-loop.ll
> > > URL: http://llvm.org/viewvc/llvm-
> > > project/llvm/trunk/test/Transforms/LoopUnroll/peel-
> > > loop.ll?rev=324572&r1=324571&r2=324572&view=diff
> > >
> >
> ==========================================================
> > > ====================
> > > --- llvm/trunk/test/Transforms/LoopUnroll/peel-loop.ll (original)
> > > +++ llvm/trunk/test/Transforms/LoopUnroll/peel-loop.ll Wed Feb 7
> > > +++ 23:16:29 2018
> > > @@ -19,10 +19,8 @@
> > > ; CHECK: store i32 2, i32* %[[INC2]], align 4 ; CHECK:
> > > %[[CMP3:.*]] = icmp eq
> > > i32 %k, 3 ; CHECK: br i1 %[[CMP3]], label %for.end, label
> > > %[[LOOP_PH:.*]] -;
> > > CHECK: [[LOOP_PH]]:
> > > -; CHECK: br label %[[LOOP:.*]]
> > > -; CHECK: [[LOOP]]:
> > > -; CHECK: %[[IV:.*]] = phi i32 [ 3, %[[LOOP_PH]] ], [ {{.*}},
> > > %[[LOOP]] ]
> > > +; CHECK: for.end:
> > > +; CHECK: ret void
> > >
> > > define void @basic(i32* %p, i32 %k) #0 {
> > > entry:
> > > @@ -68,11 +66,8 @@ for.end:
> > > ; CHECK: store i32 2, i32* %[[INC2]], align 4 ; CHECK:
> > > %[[CMP3:.*]] = icmp eq
> > > i32 %k, 3 ; CHECK: br i1 %[[CMP3]], label %for.end, label
> > > %[[LOOP_PH:.*]] -;
> > > CHECK: [[LOOP_PH]]:
> > > -; CHECK: br label %[[LOOP:.*]]
> > > -; CHECK: [[LOOP]]:
> > > -; CHECK: %[[IV:.*]] = phi i32 [ 3, %[[LOOP_PH]] ], [ %[[IV:.*]],
> > > %[[LOOP]] ] -;
> > > CHECK: %ret = phi i32 [ 0, %entry ], [ 1, %[[NEXT0]] ], [ 2,
> > > %[[NEXT1]] ], [ 3, %[[NEXT2]] ], [ %[[IV]], %[[LOOP]] ]
> > > +; CHECK: for.end:
> > > +; CHECK: %ret = phi i32 [ 0, %entry ], [ 1, %[[NEXT0]] ], [ 2,
> > > +%[[NEXT1]] ], [ 3, %[[NEXT2]] ], [ %inc, %for.body ]
> > > ; CHECK: ret i32 %ret
> > > define i32 @output(i32* %p, i32 %k) #0 {
> > > entry:
> > >
> > > Modified: llvm/trunk/test/Transforms/LoopUnswitch/2015-06-17-
> > > Metadata.ll
> > > URL: http://llvm.org/viewvc/llvm-
> > > project/llvm/trunk/test/Transforms/LoopUnswitch/2015-06-17-
> > > Metadata.ll?rev=324572&r1=324571&r2=324572&view=diff
> > >
> >
> ==========================================================
> > > ====================
> > > --- llvm/trunk/test/Transforms/LoopUnswitch/2015-06-17-Metadata.ll
> > > (original)
> > > +++ llvm/trunk/test/Transforms/LoopUnswitch/2015-06-17-Metadata.ll
> > Wed
> > > +++ Feb 7 23:16:29 2018
> > > @@ -16,7 +16,7 @@ for.body:
> > > %cmp1 = icmp eq i32 %a, 12345
> > > br i1 %cmp1, label %if.then, label %if.else, !prof !0 ; CHECK:
> > > %cmp1 = icmp eq i32 %a, 12345 -; CHECK-NEXT: br i1 %cmp1, label
> > > %for.body.preheader.split.us, label %for.body.preheader.split, !prof
> > > !0
> > > +; CHECK-NEXT: br i1 %cmp1, label %for.body.us, label %for.body,
> > > +!prof
> > > +!0
> > > if.then: ; preds = %for.body
> > > ; CHECK: for.body.us:
> > > ; CHECK: add nsw i32 %{{.*}}, 123
> > > @@ -53,7 +53,7 @@ entry:
> > > br label %for.body
> > > ;CHECK: entry:
> > > ;CHECK-NEXT: %cmp1 = icmp eq i32 1, 2
> > > -;CHECK-NEXT: br i1 %cmp1, label %entry.split, label
> > > %for.cond.cleanup.split, !prof !1
> > > +;CHECK-NEXT: br i1 %cmp1, label %for.body, label
> > > +%for.cond.cleanup.split, !prof !1
> > > ;CHECK: for.body:
> > > for.body: ; preds = %for.inc, %entry
> > > %inc.i = phi i32 [ 0, %entry ], [ %inc, %if.then ]
> > >
> > > Modified: llvm/trunk/test/Transforms/LoopUnswitch/infinite-loop.ll
> > > URL: http://llvm.org/viewvc/llvm-
> > > project/llvm/trunk/test/Transforms/LoopUnswitch/infinite-
> > > loop.ll?rev=324572&r1=324571&r2=324572&view=diff
> > >
> >
> ==========================================================
> > > ====================
> > > --- llvm/trunk/test/Transforms/LoopUnswitch/infinite-loop.ll
> > > (original)
> > > +++ llvm/trunk/test/Transforms/LoopUnswitch/infinite-loop.ll Wed Feb
> > > +++ 7
> > > +++ 23:16:29 2018
> > > @@ -16,7 +16,7 @@
> > > ; CHECK-NEXT: br i1 %a, label %entry.split, label %abort0.split
> > >
> > > ; CHECK: entry.split:
> > > -; CHECK-NEXT: br i1 %b, label %entry.split.split, label
> > > %abort1.split
> > > +; CHECK-NEXT: br i1 %b, label %for.body, label %abort1.split
> > >
> > > ; CHECK: for.body:
> > > ; CHECK-NEXT: br label %for.body
> > >
> > > Added:
> > llvm/trunk/test/Transforms/SimplifyCFG/UncondBranchToHeader.ll
> > > URL: http://llvm.org/viewvc/llvm-
> > >
> >
> project/llvm/trunk/test/Transforms/SimplifyCFG/UncondBranchToHeader.ll
> > > ?rev=324572&view=auto
> > >
> >
> ==========================================================
> > > ====================
> > > --- llvm/trunk/test/Transforms/SimplifyCFG/UncondBranchToHeader.ll
> > > (added)
> > > +++ llvm/trunk/test/Transforms/SimplifyCFG/UncondBranchToHeader.ll
> > > Wed
> > > +++ Feb 7 23:16:29 2018
> > > @@ -0,0 +1,18 @@
> > > +; RUN: opt < %s -simplifycfg -S | FileCheck %s
> > > +
> > > +; Check that we can get rid of empty block leading to header ; if
> > > +it does not introduce new edge.
> > > +define i32 @test(i32 %c) {
> > > +entry:
> > > + br label %header
> > > +header:
> > > + %i = phi i32 [0, %entry], [%i.1, %backedge]
> > > + %i.1 = add i32 %i, 1
> > > + %cmp = icmp slt i32 %i.1, %c
> > > + br i1 %cmp, label %backedge, label %exit ; CHECK-NOT: backedge:
> > > +backedge:
> > > + br label %header
> > > +exit:
> > > + ret i32 %i
> > > +}
> > >
> > >
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at lists.llvm.org
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list