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

[Roman Lebedev via llvm-commits]

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

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