[PATCH] D89088: [MBP] Add whole chain to BlockFilterSet instead of individual BB

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added a comment.

In D89088#2348847 <https://reviews.llvm.org/D89088#2348847>, @dmajor wrote:

> In case it helps, I turned on debug logging for this pass on our repro: https://paste.mozilla.org/HBDkeGXO (starts at line 60)

The debug logging is actually very helpful! From the log we can see that bb.0 is moved after bb.2 when rotating the loop chain. So the problem is in function rotateLoop, we should check if the chain header is entry block before rotating. We should also do similar check in function rotateLoopWithProfile.

@dmajor and @aeubanks, could you help to see if the following patch can fix the crash when combined with patch adfb5415010f <https://reviews.llvm.org/rGadfb5415010fbbc009a4a6298cfda7a6ed4fa6d4>?

Thanks!

  diff --git a/llvm/lib/CodeGen/MachineBlockPlacement.cpp b/llvm/lib/CodeGen/MachineBlockPlacement.cpp
  index 8a8669638031..69f5b9d29c06 100644
  --- a/llvm/lib/CodeGen/MachineBlockPlacement.cpp
  +++ b/llvm/lib/CodeGen/MachineBlockPlacement.cpp
  @@ -2312,6 +2312,10 @@ void MachineBlockPlacement::rotateLoop(BlockChain &LoopChain,
     if (Bottom == ExitingBB)
       return;
   
  +  // The entry block should always be the first BB in a function.
  +  if (Top == &F->front())
  +    return;
  +
     bool ViableTopFallthrough = hasViableTopFallthrough(Top, LoopBlockSet);
   
     // If the header has viable fallthrough, check whether the current loop
  @@ -2386,6 +2390,11 @@ void MachineBlockPlacement::rotateLoopWithProfile(
       BlockChain &LoopChain, const MachineLoop &L,
       const BlockFilterSet &LoopBlockSet) {
     auto RotationPos = LoopChain.end();
  +  MachineBasicBlock *ChainHeaderBB = *LoopChain.begin();
  +
  +  // The entry block should always be the first BB in a function.
  +  if (ChainHeaderBB == &F->front())
  +    return;
   
     BlockFrequency SmallestRotationCost = BlockFrequency::getMaxFrequency();
   
  @@ -2404,7 +2413,6 @@ void MachineBlockPlacement::rotateLoopWithProfile(
     // chain head is not the loop header. As we only consider natural loops with
     // single header, this computation can be done only once.
     BlockFrequency HeaderFallThroughCost(0);
  -  MachineBasicBlock *ChainHeaderBB = *LoopChain.begin();
     for (auto *Pred : ChainHeaderBB->predecessors()) {
       BlockChain *PredChain = BlockToChain[Pred];
       if (!LoopBlockSet.count(Pred) &&


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89088