[llvm] r324572 - [SimplifyCFG] Re-apply Relax restriction for folding unconditional branches
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 14 17:22:52 PST 2018
On 02/14/2018 01:59 AM, David Green via llvm-commits wrote:
> 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 :) ).
Just a quick note - loop simplify form is supposed to be actively formed
where needed isn't it? If SimplifyCFG is breaking it, the next loop
pass should restore it shouldn't it?
(Or has this changed since I last looked? I know there was some churn
in this area...)
>
> 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
> _______________________________________________
> 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