[PATCH] D70030: [MachineBlockPlacement] Fix UnscheduledPredecessors counter to reflect tail duplication.

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:2976
+      continue;
+    if (&BBChain != BlockToChain[Pred] && BBChain.UnscheduledPredecessors != 0)
+      --BBChain.UnscheduledPredecessors;
----------------
huihuiz wrote:
> Carrot wrote:
> > huihuiz wrote:
> > > Carrot wrote:
> > > > Here if BBChain.UnscheduledPredecessors == 0, then there is something wrong.
> > > already guarded under "BBChain.UnscheduledPredecessors != 0"
> > You should not hide this kind of compiler internal error. Even do nothing is better than hide it. Of course a better choice is assert it.
> > 
> I don't think so. Assert here may not be good idea.
> 
> Refer to line:620, this also make sure not decrementing on an already zero UnscheduledPredecessors counter.
> ```
>    // This is a cross-chain edge that is within the loop, so decrement the
>     // loop predecessor count of the destination chain.
>     if (SuccChain.UnscheduledPredecessors == 0 ||
>         --SuccChain.UnscheduledPredecessors > 0)
>       continue;
> 
> ```
> 
> Please let me know if you can agree with this? I am trying not to waste each other's time on endless arguing.
Nobody wants to argue with you for no reason! If you want your patch to be accepted quickly, please try to understand the algorithm carefully.

This code looks also want to hide something wrong, so not a good practice.

I may change my idea if you can find some reasonable situation that SuccChain.UnscheduledPredecessors <= 0 when there are still cross chain edges flow to SuccChain.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70030/new/

https://reviews.llvm.org/D70030