Skip to content

[CodeGen] Move dom tree invalidation in MBP #102453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions llvm/lib/CodeGen/MachineBlockPlacement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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++) {
Expand Down
65 changes: 65 additions & 0 deletions llvm/test/CodeGen/X86/code_placement_taildup.ll
Original file line number Diff line number Diff line change
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@aengelke aengelke Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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}
Loading