<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Apr 29, 2021 at 12:22 AM Roman Lebedev <<a href="mailto:lebedev.ri@gmail.com">lebedev.ri@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hm, indeed. I've gone through several iterations of that name,<br>
and somehow ended up with an incorrect one. Let me fix it up.<br>
<br>
Roman<br></blockquote><div><br></div><div>Thanks! Looking at this change in more detail, I think there are more issues here. The doc comment for the function says:</div><br>> We also allow one predecessor to end with conditional branch (but no more<br>> than one).</div><div class="gmail_quote"><br></div><div class="gmail_quote">and the 30-line comment directly before the changed code also repeats this, in considerably more detail.</div><div class="gmail_quote"><br></div><div class="gmail_quote">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.<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">Regards,</div><div class="gmail_quote">Nikita<br></div><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On Thu, Apr 29, 2021 at 1:16 AM Nikita Popov <<a href="mailto:nikita.ppv@gmail.com" target="_blank">nikita.ppv@gmail.com</a>> wrote:<br>
><br>
> On Thu, Apr 29, 2021 at 12:01 AM Roman Lebedev via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>><br>
>> Author: Roman Lebedev<br>
>> Date: 2021-04-29T01:01:01+03:00<br>
>> New Revision: 1886aad9d03b95c35260d6d8013d746bd39dc94a<br>
>><br>
>> URL: <a href="https://github.com/llvm/llvm-project/commit/1886aad9d03b95c35260d6d8013d746bd39dc94a" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/1886aad9d03b95c35260d6d8013d746bd39dc94a</a><br>
>> DIFF: <a href="https://github.com/llvm/llvm-project/commit/1886aad9d03b95c35260d6d8013d746bd39dc94a.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/1886aad9d03b95c35260d6d8013d746bd39dc94a.diff</a><br>
>><br>
>> LOG: [SimplifyCFG] Common code sinking: relax restriction on non-uncond predecessors<br>
>><br>
>> While we have a known profitability issue for sinking in presence of<br>
>> non-unconditional predecessors, there isn't any known issues<br>
>> for having multiple such non-unconditional predecessors,<br>
>> so said restriction appears to be artificial. Lift it.<br>
>><br>
>> Added:<br>
>><br>
>><br>
>> Modified:<br>
>> llvm/lib/Transforms/Utils/SimplifyCFG.cpp<br>
>> llvm/test/Transforms/SimplifyCFG/X86/sink-common-code.ll<br>
>><br>
>> Removed:<br>
>><br>
>><br>
>><br>
>> ################################################################################<br>
>> diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp<br>
>> index 77c0890fa91e..0704c5f2321a 100644<br>
>> --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp<br>
>> +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp<br>
>> @@ -1988,15 +1988,13 @@ static bool SinkCommonCodeFromPredecessors(BasicBlock *BB,<br>
>> // [ end ]<br>
>> //<br>
>> SmallVector<BasicBlock*,4> UnconditionalPreds;<br>
>> - Instruction *Cond = nullptr;<br>
>> - for (auto *B : predecessors(BB)) {<br>
>> - auto *T = B->getTerminator();<br>
>> - if (isa<BranchInst>(T) && cast<BranchInst>(T)->isUnconditional())<br>
>> - UnconditionalPreds.push_back(B);<br>
>> - else if ((isa<BranchInst>(T) || isa<SwitchInst>(T)) && !Cond)<br>
>> - Cond = T;<br>
>> + bool AllPredsAreUnconditional = false;<br>
>> + for (auto *PredBB : predecessors(BB)) {<br>
>> + auto *PredBr = dyn_cast<BranchInst>(PredBB->getTerminator());<br>
>> + if (PredBr && PredBr->isUnconditional())<br>
>> + UnconditionalPreds.push_back(PredBB);<br>
>> else<br>
>> - return false;<br>
>> + AllPredsAreUnconditional = true;<br>
><br>
><br>
> Is the name of this variable inverted? You're setting it to true when there is a conditional branch.<br>
><br>
> Nikita<br>
><br>
>><br>
>> }<br>
>> if (UnconditionalPreds.size() < 2)<br>
>> return false;<br>
>> @@ -2063,7 +2061,7 @@ static bool SinkCommonCodeFromPredecessors(BasicBlock *BB,<br>
>><br>
>> bool Changed = false;<br>
>><br>
>> - if (Cond) {<br>
>> + if (AllPredsAreUnconditional) {<br>
>> // It is always legal to sink common instructions from unconditional<br>
>> // predecessors. However, if not all predecessors are unconditional,<br>
>> // this transformation might be pessimizing. So as a rule of thumb,<br>
>><br>
>> diff --git a/llvm/test/Transforms/SimplifyCFG/X86/sink-common-code.ll b/llvm/test/Transforms/SimplifyCFG/X86/sink-common-code.ll<br>
>> index ade36c7019d1..a85ad8e69a20 100644<br>
>> --- a/llvm/test/Transforms/SimplifyCFG/X86/sink-common-code.ll<br>
>> +++ b/llvm/test/Transforms/SimplifyCFG/X86/sink-common-code.ll<br>
>> @@ -1538,14 +1538,11 @@ define void @multiple_cond_preds(i1 %c0, i1 %c1, i1 %c2) {<br>
>> ; CHECK-NEXT: br i1 [[C0:%.*]], label [[DISPATCH1:%.*]], label [[DISPATCH2:%.*]]<br>
>> ; CHECK: dispatch1:<br>
>> ; CHECK-NEXT: call void @direct_callee2()<br>
>> -; CHECK-NEXT: br i1 [[C1:%.*]], label [[UNCOND_PRED0:%.*]], label [[END:%.*]]<br>
>> +; CHECK-NEXT: br i1 [[C1:%.*]], label [[END_SINK_SPLIT:%.*]], label [[END:%.*]]<br>
>> ; CHECK: dispatch2:<br>
>> ; CHECK-NEXT: call void @direct_callee3()<br>
>> -; CHECK-NEXT: br i1 [[C2:%.*]], label [[UNCOND_PRED1:%.*]], label [[END]]<br>
>> -; CHECK: uncond_pred0:<br>
>> -; CHECK-NEXT: call void @direct_callee()<br>
>> -; CHECK-NEXT: br label [[END]]<br>
>> -; CHECK: uncond_pred1:<br>
>> +; CHECK-NEXT: br i1 [[C2:%.*]], label [[END_SINK_SPLIT]], label [[END]]<br>
>> +; CHECK: end.sink.split:<br>
>> ; CHECK-NEXT: call void @direct_callee()<br>
>> ; CHECK-NEXT: br label [[END]]<br>
>> ; CHECK: end:<br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>