Skip to content

[RegisterCoalescer] Clear instructions not recorded in ErasedInstrs but erased #79820

New issue

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

#

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

Already on GitHub? # to your account

Merged
merged 5 commits into from
Feb 9, 2024
Merged

[RegisterCoalescer] Clear instructions not recorded in ErasedInstrs but erased #79820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
646c95f
Pre-commit test cases
dianqk Feb 8, 2024
00254d3
[RegisterCoalescer] Deferring deletion of instructions in ErasedInstr…
dianqk Feb 8, 2024
4e0d1c2
fixup! Pre-commit test cases
dianqk Feb 8, 2024
0821515
[RegisterCoalescer] Clear instructions not recorded in `ErasedInstrs`…
dianqk Feb 9, 2024
a27d5b3
fixup! [RegisterCoalescer] Clear instructions not recorded in `Erased…
dianqk Feb 9, 2024
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
27 changes: 22 additions & 5 deletions llvm/lib/CodeGen/RegisterCoalescer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,8 @@ namespace {
/// was successfully coalesced away. If it is not currently possible to
/// coalesce this interval, but it may be possible if other things get
/// coalesced, then it returns true by reference in 'Again'.
bool joinCopy(MachineInstr *CopyMI, bool &Again);
bool joinCopy(MachineInstr *CopyMI, bool &Again,
SmallPtrSetImpl<MachineInstr *> &CurrentErasedInstrs);

/// Attempt to join these two intervals. On failure, this
/// returns false. The output "SrcInt" will not have been modified, so we
Expand Down Expand Up @@ -1964,7 +1965,9 @@ void RegisterCoalescer::setUndefOnPrunedSubRegUses(LiveInterval &LI,
LIS->shrinkToUses(&LI);
}

bool RegisterCoalescer::joinCopy(MachineInstr *CopyMI, bool &Again) {
bool RegisterCoalescer::joinCopy(
MachineInstr *CopyMI, bool &Again,
SmallPtrSetImpl<MachineInstr *> &CurrentErasedInstrs) {
Again = false;
LLVM_DEBUG(dbgs() << LIS->getInstructionIndex(*CopyMI) << '\t' << *CopyMI);

Expand Down Expand Up @@ -2156,7 +2159,9 @@ bool RegisterCoalescer::joinCopy(MachineInstr *CopyMI, bool &Again) {
// CopyMI has been erased by joinIntervals at this point. Remove it from
// ErasedInstrs since copyCoalesceWorkList() won't add a successful join back
// to the work list. This keeps ErasedInstrs from growing needlessly.
ErasedInstrs.erase(CopyMI);
if (ErasedInstrs.erase(CopyMI))
// But we may encounter the instruction again in this iteration.
CurrentErasedInstrs.insert(CopyMI);
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we end up with the same copy instruction twice in the worklist?

Copy link
Member Author

Choose a reason for hiding this comment

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

From the debugging results, it looks like removePartialRedundancy changes the contents of the work list.


// Rewrite all SrcReg operands to DstReg.
// Also update DstReg operands to include DstIdx if it is set.
Expand Down Expand Up @@ -3982,21 +3987,33 @@ void RegisterCoalescer::lateLiveIntervalUpdate() {
bool RegisterCoalescer::
copyCoalesceWorkList(MutableArrayRef<MachineInstr*> CurrList) {
bool Progress = false;
SmallPtrSet<MachineInstr *, 4> CurrentErasedInstrs;
for (MachineInstr *&MI : CurrList) {
if (!MI)
continue;
// Skip instruction pointers that have already been erased, for example by
// dead code elimination.
if (ErasedInstrs.count(MI)) {
if (ErasedInstrs.count(MI) || CurrentErasedInstrs.count(MI)) {
MI = nullptr;
continue;
}
bool Again = false;
bool Success = joinCopy(MI, Again);
bool Success = joinCopy(MI, Again, CurrentErasedInstrs);
Progress |= Success;
if (Success || !Again)
MI = nullptr;
}
// Clear instructions not recorded in `ErasedInstrs` but erased.
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole thing feels overcomplicated but I don't have any better suggestion

Copy link
Member Author

@dianqk dianqk Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think all because we are dancing with dangling pointers.
🔩🔩🔩
🔩💃🔩
🔩🔩🔩

if (!CurrentErasedInstrs.empty()) {
for (MachineInstr *&MI : CurrList) {
if (MI && CurrentErasedInstrs.count(MI))
MI = nullptr;
}
for (MachineInstr *&MI : WorkList) {
if (MI && CurrentErasedInstrs.count(MI))
MI = nullptr;
}
}
return Progress;
}

Expand Down
213 changes: 213 additions & 0 deletions llvm/test/CodeGen/LoongArch/register-coalescer-crash-pr79718.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 4
# RUN: llc -o - %s -mtriple=loongarch64 \
# RUN: -run-pass=register-coalescer -join-liveintervals=1 -join-splitedges=0 | FileCheck %s

---
name: foo
tracksRegLiveness: true
body: |
; CHECK-LABEL: name: foo
; CHECK: bb.0:
; CHECK-NEXT: successors: %bb.1(0x80000000)
; CHECK-NEXT: liveins: $r4, $r5, $r6, $r7, $r8
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $r8
; CHECK-NEXT: [[COPY1:%[0-9]+]]:gpr = COPY $r7
; CHECK-NEXT: [[COPY2:%[0-9]+]]:gpr = COPY $r6
; CHECK-NEXT: [[COPY3:%[0-9]+]]:gpr = COPY $r5
; CHECK-NEXT: [[COPY4:%[0-9]+]]:gpr = COPY $r4
; CHECK-NEXT: [[ANDI:%[0-9]+]]:gpr = ANDI [[COPY3]], 1
; CHECK-NEXT: [[ORI:%[0-9]+]]:gpr = ORI $r0, 1
; CHECK-NEXT: [[ANDI1:%[0-9]+]]:gpr = ANDI [[COPY2]], 1
; CHECK-NEXT: [[ANDI2:%[0-9]+]]:gpr = ANDI [[COPY1]], 1
; CHECK-NEXT: [[ANDI3:%[0-9]+]]:gpr = ANDI [[COPY]], 1
; CHECK-NEXT: [[COPY5:%[0-9]+]]:gpr = COPY $r0
; CHECK-NEXT: [[COPY6:%[0-9]+]]:gpr = COPY $r0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.1:
; CHECK-NEXT: successors: %bb.2(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY7:%[0-9]+]]:gpr = COPY [[COPY5]]
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.2:
; CHECK-NEXT: successors: %bb.3(0x40000000), %bb.4(0x40000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: BEQZ [[ANDI]], %bb.4
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.3:
; CHECK-NEXT: successors: %bb.9(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: PseudoBR %bb.9
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.4:
; CHECK-NEXT: successors: %bb.5(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.5:
; CHECK-NEXT: successors: %bb.7(0x7c000000), %bb.6(0x04000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: dead [[LD_D:%[0-9]+]]:gpr = LD_D $r0, 8
; CHECK-NEXT: dead [[LD_D1:%[0-9]+]]:gpr = LD_D $r0, 0
; CHECK-NEXT: BNEZ [[ANDI1]], %bb.7
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.6:
; CHECK-NEXT: successors: %bb.11(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY6:%[0-9]+]]:gpr = COPY $r0
; CHECK-NEXT: PseudoBR %bb.11
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.7:
; CHECK-NEXT: successors: %bb.8(0x7c000000), %bb.10(0x04000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: BEQZ [[ANDI2]], %bb.10
; CHECK-NEXT: PseudoBR %bb.8
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.8:
; CHECK-NEXT: successors: %bb.9(0x04000000), %bb.5(0x7c000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY6:%[0-9]+]]:gpr = ADDI_D [[COPY6]], 1
; CHECK-NEXT: BEQZ [[ANDI3]], %bb.5
; CHECK-NEXT: PseudoBR %bb.9
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.9:
; CHECK-NEXT: successors: %bb.12(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: ST_B $r0, [[COPY4]], 0
; CHECK-NEXT: PseudoBR %bb.12
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.10:
; CHECK-NEXT: successors: %bb.11(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: [[COPY5:%[0-9]+]]:gpr = ADDI_D [[COPY6]], 1
; CHECK-NEXT: [[COPY6:%[0-9]+]]:gpr = COPY [[ORI]]
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.11:
; CHECK-NEXT: successors: %bb.12(0x80000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: ST_D $r0, [[COPY4]], 0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: bb.12:
; CHECK-NEXT: successors: %bb.2(0x7c000000), %bb.1(0x04000000)
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: BEQ [[COPY7]], [[ORI]], %bb.2
; CHECK-NEXT: PseudoBR %bb.1
bb.0:
liveins: $r4, $r5, $r6, $r7, $r8

%0:gpr = COPY killed $r8
%1:gpr = COPY killed $r7
%2:gpr = COPY killed $r6
%3:gpr = COPY killed $r5
%4:gpr = COPY killed $r4
%5:gpr = COPY $r0
%6:gpr = COPY killed %5
%7:gpr = ANDI killed %3, 1
%8:gpr = ORI $r0, 1
%9:gpr = ANDI killed %2, 1
%10:gpr = ANDI killed %1, 1
%11:gpr = ANDI killed %0, 1
%12:gpr = COPY %6
%13:gpr = COPY killed %6
%14:gpr = IMPLICIT_DEF

bb.1:
%15:gpr = COPY killed %14
%16:gpr = COPY killed %13
%17:gpr = COPY killed %12
%18:gpr = COPY %17
%19:gpr = COPY %16
%20:gpr = COPY killed %16
%21:gpr = COPY killed %15

bb.2:
successors: %bb.3, %bb.4

%22:gpr = COPY killed %21
%23:gpr = COPY killed %20
%24:gpr = COPY killed %19
%25:gpr = COPY killed %18
BEQZ %7, %bb.4

bb.3:
%26:gpr = COPY killed %24
%27:gpr = COPY killed %23
PseudoBR %bb.9

bb.4:
%28:gpr = COPY killed %23

bb.5:
successors: %bb.7(0x7c000000), %bb.6(0x04000000)

%29:gpr = COPY killed %28
dead %30:gpr = LD_D $r0, 8
dead %31:gpr = LD_D $r0, 0
BNEZ %9, %bb.7

bb.6:
%32:gpr = COPY $r0
%33:gpr = COPY killed %32
%34:gpr = COPY killed %33
%35:gpr = COPY killed %22
PseudoBR %bb.11

bb.7:
successors: %bb.8(0x7c000000), %bb.10(0x04000000)

BEQZ %10, %bb.10
PseudoBR %bb.8

bb.8:
successors: %bb.9(0x04000000), %bb.5(0x7c000000)

%36:gpr = ADDI_D killed %29, 1
%28:gpr = COPY %36
%26:gpr = COPY %36
%27:gpr = COPY killed %36
BEQZ %11, %bb.5
PseudoBR %bb.9

bb.9:
%37:gpr = COPY killed %27
%38:gpr = COPY killed %26
%39:gpr = COPY $r0
ST_B killed %39, %4, 0
%40:gpr = COPY killed %25
%41:gpr = COPY killed %38
%42:gpr = COPY killed %37
%43:gpr = COPY killed %22
PseudoBR %bb.12

bb.10:
%44:gpr = ADDI_D killed %29, 1
%34:gpr = COPY %8
%35:gpr = COPY killed %44

bb.11:
%45:gpr = COPY killed %35
%46:gpr = COPY killed %34
%47:gpr = COPY $r0
ST_D killed %47, %4, 0
%40:gpr = COPY %45
%41:gpr = COPY %46
%42:gpr = COPY killed %46
%43:gpr = COPY killed %45

bb.12:
successors: %bb.2(0x7c000000), %bb.1(0x04000000)

%48:gpr = COPY killed %43
%49:gpr = COPY killed %42
%50:gpr = COPY killed %41
%51:gpr = COPY killed %40
%12:gpr = COPY %51
%13:gpr = COPY %50
%14:gpr = COPY %48
%18:gpr = COPY killed %51
%19:gpr = COPY killed %50
%20:gpr = COPY killed %49
%21:gpr = COPY killed %48
BEQ %17, %8, %bb.2
PseudoBR %bb.1

...
103 changes: 103 additions & 0 deletions llvm/test/CodeGen/X86/PR71178-register-coalescer-crash.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
; RUN: llc < %s -mtriple=x86_64 -- | FileCheck %s

define i32 @h(i1 %arg, i32 %arg1) {
; CHECK-LABEL: h:
; CHECK: # %bb.0: # %bb
; CHECK-NEXT: movl $1, %eax
; CHECK-NEXT: movabsq $9166129423, %rcx # imm = 0x22258090F
; CHECK-NEXT: xorl %edx, %edx
; CHECK-NEXT: jmp .LBB0_1
; CHECK-NEXT: .p2align 4, 0x90
; CHECK-NEXT: .LBB0_9: # %bb18
; CHECK-NEXT: # in Loop: Header=BB0_1 Depth=1
; CHECK-NEXT: xorl %eax, %eax
; CHECK-NEXT: testb $1, %dil
; CHECK-NEXT: jne .LBB0_10
; CHECK-NEXT: .LBB0_1: # %bb4
; CHECK-NEXT: # =>This Inner Loop Header: Depth=1
; CHECK-NEXT: testq %rdx, %rdx
; CHECK-NEXT: jne .LBB0_2
; CHECK-NEXT: # %bb.7: # %bb16
; CHECK-NEXT: # in Loop: Header=BB0_1 Depth=1
; CHECK-NEXT: testb $1, %dil
; CHECK-NEXT: jne .LBB0_9
; CHECK-NEXT: # %bb.8: # %bb17
; CHECK-NEXT: # in Loop: Header=BB0_1 Depth=1
; CHECK-NEXT: movq %rcx, %rdx
; CHECK-NEXT: jmp .LBB0_9
; CHECK-NEXT: .LBB0_2: # %bb9
; CHECK-NEXT: # in Loop: Header=BB0_1 Depth=1
; CHECK-NEXT: testb $1, %dil
; CHECK-NEXT: testb $1, %dil
; CHECK-NEXT: je .LBB0_4
; CHECK-NEXT: # %bb.3: # %bb13
; CHECK-NEXT: # in Loop: Header=BB0_1 Depth=1
; CHECK-NEXT: xorl %eax, %eax
; CHECK-NEXT: .LBB0_4: # %bb14
; CHECK-NEXT: # in Loop: Header=BB0_1 Depth=1
; CHECK-NEXT: cmpl $1, %esi
; CHECK-NEXT: je .LBB0_1
; CHECK-NEXT: # %bb.5: # %bb14
; CHECK-NEXT: movl %eax, %r8d
; CHECK-NEXT: testl %esi, %esi
; CHECK-NEXT: movl %esi, %eax
; CHECK-NEXT: jne .LBB0_6
; CHECK-NEXT: .LBB0_10: # %bb22
; CHECK-NEXT: retq
; CHECK-NEXT: .LBB0_6: # %bb22.loopexit1
; CHECK-NEXT: movl %r8d, %eax
; CHECK-NEXT: retq
bb:
br label %bb2

bb2: ; preds = %bb14, %bb
%i = phi i64 [ %i5, %bb14 ], [ 0, %bb ]
%i3 = phi i32 [ %i15, %bb14 ], [ 1, %bb ]
br label %bb4

bb4: ; preds = %bb18, %bb2
%i5 = phi i64 [ %i19, %bb18 ], [ %i, %bb2 ]
%i6 = phi i64 [ %i20, %bb18 ], [ %i, %bb2 ]
%i7 = phi i32 [ 0, %bb18 ], [ %i3, %bb2 ]
%i8 = icmp eq i64 %i6, 0
br i1 %i8, label %bb16, label %bb9

bb9: ; preds = %bb4
br i1 %arg, label %bb12, label %bb10

bb10: ; preds = %bb9
%i11 = sdiv i64 0, 0
br label %bb12

bb12: ; preds = %bb10, %bb9
br i1 %arg, label %bb13, label %bb14

bb13: ; preds = %bb12
br label %bb14

bb14: ; preds = %bb13, %bb12
%i15 = phi i32 [ 0, %bb13 ], [ %i7, %bb12 ]
switch i32 %arg1, label %bb22 [
i32 0, label %bb21
i32 1, label %bb2
]

bb16: ; preds = %bb4
br i1 %arg, label %bb18, label %bb17

bb17: ; preds = %bb16
br label %bb18

bb18: ; preds = %bb17, %bb16
%i19 = phi i64 [ 9166129423, %bb17 ], [ %i5, %bb16 ]
%i20 = phi i64 [ 9166129423, %bb17 ], [ %i6, %bb16 ]
br i1 %arg, label %bb22, label %bb4

bb21: ; preds = %bb14
br label %bb22

bb22: ; preds = %bb21, %bb18, %bb14
%i23 = phi i32 [ %arg1, %bb21 ], [ %i15, %bb14 ], [ 0, %bb18 ]
ret i32 %i23
}