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

Thu Apr 29 12:09:04 PDT 2021

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. But given how explicit they
were about the limitation, I'm wondering if it was really an arbitrary one.

Regards,
Nikita

>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210429/a656555d/attachment.html>