[CodeGen] Move dom tree invalidation in MBP

[aengelke]

This is not the correct fix. It is just a less ugly version of #102427. Also, add a test case for #102427.

Unfortunately, this looks broken beyond a level that I can easily fix (I don't really understand the code). It seems that it does some updates without updating the MPDT, which is still used afterwards. I think the use of invalid MPDTs was introduced in https://reviews.llvm.org/D28583, but I haven't validated this in detail.

cc @iteratee

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: Alexis Engelke (aengelke)

Changes

This is not the correct fix. It is just a less ugly version of #102427. Also, add a test case for #102427.

Unfortunately, this looks broken beyond a level that I can easily fix (I don't really understand the code). It seems that it does some updates without updating the MPDT, which is still used afterwards. I think the use of invalid MPDTs was introduced in https://reviews.llvm.org/D28583, but I haven't validated this in detail.

cc @iteratee

---

Full diff: https://github.com/llvm/llvm-project/pull/102453.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/MachineBlockPlacement.cpp (+10-5)
• (added) llvm/test/CodeGen/X86/code_placement_taildup.ll (+65)

diff --git a/llvm/lib/CodeGen/MachineBlockPlacement.cpp b/llvm/lib/CodeGen/MachineBlockPlacement.cpp
index be783bc4e2973..69b78cfafdc3c 100644
--- a/llvm/lib/CodeGen/MachineBlockPlacement.cpp
+++ b/llvm/lib/CodeGen/MachineBlockPlacement.cpp
@@ -1668,6 +1668,12 @@ MachineBlockPlacement::selectBestSuccessor(
     BestSucc.BB = Succ;
     BestProb = SuccProb;
   }
+
+  // TODO: isProfitableToTailDup requries a MPDT, but it is not necessarily
+  // valid at this point, because we already modified the CFG. This assertion
+  // fails.
+  // assert(MPDT && MPDT->verify());
+
   // Handle the tail duplication candidates in order of decreasing probability.
   // Stop at the first one that is profitable. Also stop if they are less
   // profitable than BestSucc. Position is important because we preserve it and
@@ -3193,6 +3199,10 @@ bool MachineBlockPlacement::maybeTailDuplicateBlock(
           }
         }
 
+        // We removed a block that possibly post-dominated other blocks. As the
+        // post-dominator tree is now invalid, clear it.
+        if (MPDT)
+          MPDT = nullptr;
         // Remove the block from loop info.
         MLI->removeBlock(RemBB);
         if (RemBB == PreferredLoopExit)
@@ -3649,11 +3659,6 @@ void MachineBlockPlacement::assignBlockOrder(
     const std::vector<const MachineBasicBlock *> &NewBlockOrder) {
   assert(F->size() == NewBlockOrder.size() && "Incorrect size of block order");
   F->RenumberBlocks();
-  // At this point, we possibly removed blocks from the function, so we can't
-  // renumber the domtree. At this point, we don't need it anymore, though.
-  // TODO: move this to the point where the dominator tree is actually
-  // invalidated (i.e., where blocks are removed without updating the domtree).
-  MPDT = nullptr;
 
   bool HasChanges = false;
   for (size_t I = 0; I < NewBlockOrder.size(); I++) {
diff --git a/llvm/test/CodeGen/X86/code_placement_taildup.ll b/llvm/test/CodeGen/X86/code_placement_taildup.ll
new file mode 100644
index 0000000000000..2aa057d56e0c1
--- /dev/null
+++ b/llvm/test/CodeGen/X86/code_placement_taildup.ll
@@ -0,0 +1,65 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=x86_64 -enable-ext-tsp-block-placement=1 < %s | FileCheck %s
+
+; Test that dominator tree is invalidated after block removals.
+
+define fastcc i1 @fn(i1 %0) {
+; CHECK-LABEL: fn:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    testb $1, %dil
+; CHECK-NEXT:    je .LBB0_3
+; CHECK-NEXT:  # %bb.1:
+; CHECK-NEXT:    testb $1, %dil
+; CHECK-NEXT:    jne .LBB0_2
+; CHECK-NEXT:  .LBB0_3:
+; CHECK-NEXT:    xorl %eax, %eax
+; CHECK-NEXT:    # kill: def $al killed $al killed $eax
+; CHECK-NEXT:    retq
+; CHECK-NEXT:  .LBB0_2:
+; CHECK-NEXT:    movb $1, %al
+; CHECK-NEXT:    # kill: def $al killed $al killed $eax
+; CHECK-NEXT:    retq
+  br i1 %0, label %2, label %4
+
+2:                                                ; preds = %1
+  br i1 %0, label %3, label %4
+
+3:                                                ; preds = %2
+  br label %4
+
+4:                                                ; preds = %3, %2, %1
+  %5 = phi i1 [ true, %3 ], [ false, %1 ], [ false, %2 ]
+  ret i1 %5
+}
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 1, !"ProfileSummary", !1}
+!1 = !{!2, !3, !4, !5, !6, !7, !8, !9, !10, !11}
+!2 = !{!"ProfileFormat", !"InstrProf"}
+!3 = !{!"TotalCount", i64 87557062425}
+!4 = !{!"MaxCount", i64 626551227}
+!5 = !{!"MaxInternalCount", i64 626551227}
+!6 = !{!"MaxFunctionCount", i64 510580166}
+!7 = !{!"NumCounts", i64 5667726}
+!8 = !{!"NumFunctions", i64 963638}
+!9 = !{!"IsPartialProfile", i64 0}
+!10 = !{!"PartialProfileRatio", double 0.000000e+00}
+!11 = !{!"DetailedSummary", !12}
+!12 = !{!13, !14, !15, !16, !17, !18, !19, !20, !21, !22, !23, !24, !25, !26, !27, !28}
+!13 = !{i32 10000, i64 510580166, i32 2}
+!14 = !{i32 100000, i64 101501872, i32 50}
+!15 = !{i32 200000, i64 33917881, i32 208}
+!16 = !{i32 300000, i64 19300443, i32 554}
+!17 = !{i32 400000, i64 10742891, i32 1176}
+!18 = !{i32 500000, i64 6614715, i32 2229}
+!19 = !{i32 600000, i64 4208085, i32 3894}
+!20 = !{i32 700000, i64 2318291, i32 6730}
+!21 = !{i32 800000, i64 1208084, i32 11969}
+!22 = !{i32 900000, i64 444413, i32 23829}
+!23 = !{i32 950000, i64 174449, i32 39277}
+!24 = !{i32 990000, i64 20453, i32 93641}
+!25 = !{i32 999000, i64 1534, i32 213507}
+!26 = !{i32 999900, i64 120, i32 367707}
+!27 = !{i32 999990, i64 18, i32 565769}
+!28 = !{i32 999999, i64 4, i32 632473}

[david-xl]

How is it tested? Does the test trigger assertion without the fix (not able to reproduce it)?

[aengelke]

The test should trigger the assertion without the fix and without the MPDT = nullptr change from #102427 in assignBlockOrder.