[llvm] r324572 - [SimplifyCFG] Re-apply Relax restriction for folding unconditional branches
David Green via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 14 01:59:05 PST 2018
Hello
My understanding of a preheader is essentially something we can hoist into. So it needs a single successor - the loop header. If we have code such as this:
entry:
...
br %cond, label %preheader, label %else
preheader:
br %loop
loop:
...
br %loopcond, label %loop, label %end
else:
...
Then if we remove the "preheader" (as it has no instructions, one pred and one succ), the loop will have a predecessor ("entry") with two successors.
So as far as I can tell, this change does break loop simple form. Correct me if I am wrong here. I will try and look into the extact cause of the regressions when I get time, but this seems incorrect either way (and I worry the differences there are subtle and difficult to fix - different code into the backend can produce different output. Big shocker :) ).
Don't get me wrong, I like the idea of removing the extra blocks in the loops, that sounds like a good simplification. But if it's removing preheaders too, it doesn't seem quite correct as-is.
Dave
From: Serguei Katkov <serguei.katkov at azul.com>
Sent: 14 February 2018 08:32
To: Serguei Katkov; David Green
Cc: nd; llvm-commits
Subject: RE: [llvm] r324572 - [SimplifyCFG] Re-apply Relax restriction for folding unconditional branches
If you end up with restriction to simplify pre-headers then the less intrusive way I see:
1) In the function iterativelySimplifyCFG collect not only loop headers but also backedges.
2) Pass backedges to utility functoin
3) In utility function: if backadges are available and BB coming to header is not a backedge then prohibit its elimination.
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:01 PM
> To: David Green <David.Green at arm.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
>
> 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
> _______________________________________________
> 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