[llvm] 1886aad - [SimplifyCFG] Common code sinking: relax restriction on non-uncond predecessors

[Roman Lebedev via llvm-commits]

On Thu, Apr 29, 2021 at 10:09 PM Nikita Popov <nikita.ppv at gmail.com> wrote:
>
> On Thu, Apr 29, 2021 at 12:22 AM Roman Lebedev <lebedev.ri at gmail.com> wrote:
>>
>> Hm, indeed. I've gone through several iterations of that name,
>> and somehow ended up with an incorrect one. Let me fix it up.
>>
>> Roman
>
>
> Thanks! Looking at this change in more detail, I think there are more issues here. The doc comment for the function says:
>
> > We also allow one predecessor to end with conditional branch (but no more
> > than one).
>
> and the 30-line comment directly before the changed code also repeats this, in considerably more detail.
>
> At least those comments need to be updated.
Will do.

> But given how explicit they were about the limitation, I'm wondering if it was really an arbitrary one.
If you can find evidence contrary to that conclusion please let me know.

> Regards,
> Nikita
Roman

>> On Thu, Apr 29, 2021 at 1:16 AM Nikita Popov <nikita.ppv at gmail.com> wrote:
>> >
>> > On Thu, Apr 29, 2021 at 12:01 AM Roman Lebedev via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>> >>
>> >>
>> >> Author: Roman Lebedev
>> >> Date: 2021-04-29T01:01:01+03:00
>> >> New Revision: 1886aad9d03b95c35260d6d8013d746bd39dc94a
>> >>
>> >> URL: https://github.com/llvm/llvm-project/commit/1886aad9d03b95c35260d6d8013d746bd39dc94a
>> >> DIFF: https://github.com/llvm/llvm-project/commit/1886aad9d03b95c35260d6d8013d746bd39dc94a.diff
>> >>
>> >> LOG: [SimplifyCFG] Common code sinking: relax restriction on non-uncond predecessors
>> >>
>> >> While we have a known profitability issue for sinking in presence of
>> >> non-unconditional predecessors, there isn't any known issues
>> >> for having multiple such non-unconditional predecessors,
>> >> so said restriction appears to be artificial. Lift it.
>> >>
>> >> Added:
>> >>
>> >>
>> >> Modified:
>> >>     llvm/lib/Transforms/Utils/SimplifyCFG.cpp
>> >>     llvm/test/Transforms/SimplifyCFG/X86/sink-common-code.ll
>> >>
>> >> Removed:
>> >>
>> >>
>> >>
>> >> ################################################################################
>> >> diff  --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
>> >> index 77c0890fa91e..0704c5f2321a 100644
>> >> --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
>> >> +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
>> >> @@ -1988,15 +1988,13 @@ static bool SinkCommonCodeFromPredecessors(BasicBlock *BB,
>> >>    //         [ end ]
>> >>    //
>> >>    SmallVector<BasicBlock*,4> UnconditionalPreds;
>> >> -  Instruction *Cond = nullptr;
>> >> -  for (auto *B : predecessors(BB)) {
>> >> -    auto *T = B->getTerminator();
>> >> -    if (isa<BranchInst>(T) && cast<BranchInst>(T)->isUnconditional())
>> >> -      UnconditionalPreds.push_back(B);
>> >> -    else if ((isa<BranchInst>(T) || isa<SwitchInst>(T)) && !Cond)
>> >> -      Cond = T;
>> >> +  bool AllPredsAreUnconditional = false;
>> >> +  for (auto *PredBB : predecessors(BB)) {
>> >> +    auto *PredBr = dyn_cast<BranchInst>(PredBB->getTerminator());
>> >> +    if (PredBr && PredBr->isUnconditional())
>> >> +      UnconditionalPreds.push_back(PredBB);
>> >>      else
>> >> -      return false;
>> >> +      AllPredsAreUnconditional = true;
>> >
>> >
>> > Is the name of this variable inverted? You're setting it to true when there is a conditional branch.
>> >
>> > Nikita
>> >
>> >>
>> >>    }
>> >>    if (UnconditionalPreds.size() < 2)
>> >>      return false;
>> >> @@ -2063,7 +2061,7 @@ static bool SinkCommonCodeFromPredecessors(BasicBlock *BB,
>> >>
>> >>    bool Changed = false;
>> >>
>> >> -  if (Cond) {
>> >> +  if (AllPredsAreUnconditional) {
>> >>      // It is always legal to sink common instructions from unconditional
>> >>      // predecessors. However, if not all predecessors are unconditional,
>> >>      // this transformation might be pessimizing. So as a rule of thumb,
>> >>
>> >> diff  --git a/llvm/test/Transforms/SimplifyCFG/X86/sink-common-code.ll b/llvm/test/Transforms/SimplifyCFG/X86/sink-common-code.ll
>> >> index ade36c7019d1..a85ad8e69a20 100644
>> >> --- a/llvm/test/Transforms/SimplifyCFG/X86/sink-common-code.ll
>> >> +++ b/llvm/test/Transforms/SimplifyCFG/X86/sink-common-code.ll
>> >> @@ -1538,14 +1538,11 @@ define void @multiple_cond_preds(i1 %c0, i1 %c1, i1 %c2) {
>> >>  ; CHECK-NEXT:    br i1 [[C0:%.*]], label [[DISPATCH1:%.*]], label [[DISPATCH2:%.*]]
>> >>  ; CHECK:       dispatch1:
>> >>  ; CHECK-NEXT:    call void @direct_callee2()
>> >> -; CHECK-NEXT:    br i1 [[C1:%.*]], label [[UNCOND_PRED0:%.*]], label [[END:%.*]]
>> >> +; CHECK-NEXT:    br i1 [[C1:%.*]], label [[END_SINK_SPLIT:%.*]], label [[END:%.*]]
>> >>  ; CHECK:       dispatch2:
>> >>  ; CHECK-NEXT:    call void @direct_callee3()
>> >> -; CHECK-NEXT:    br i1 [[C2:%.*]], label [[UNCOND_PRED1:%.*]], label [[END]]
>> >> -; CHECK:       uncond_pred0:
>> >> -; CHECK-NEXT:    call void @direct_callee()
>> >> -; CHECK-NEXT:    br label [[END]]
>> >> -; CHECK:       uncond_pred1:
>> >> +; CHECK-NEXT:    br i1 [[C2:%.*]], label [[END_SINK_SPLIT]], label [[END]]
>> >> +; CHECK:       end.sink.split:
>> >>  ; CHECK-NEXT:    call void @direct_callee()
>> >>  ; CHECK-NEXT:    br label [[END]]
>> >>  ; CHECK:       end:
>> >>
>> >>
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at lists.llvm.org
>> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits