[RegisterCoalescer] Clear instructions not recorded in ErasedInstrs but erased
#79820
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-backend-loongarch Author: Quentin Dian (DianQK) ChangesFixes #79718. The same instructions may exist in an iteration. We cannot immediately delete instructions in Here's my debug logs, you can see that the instructions that are about to be processed have appeared in the work list ahead of time. This change is probably due to <details><summary>Address sanitizer logs</summary> </p> Full diff: https://github.com/llvm/llvm-project/pull/79820.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp
index cbb1a74049fbd7b..d3125fba67206b2 100644
--- a/llvm/lib/CodeGen/RegisterCoalescer.cpp
+++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp
@@ -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
@@ -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);
@@ -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);
// Rewrite all SrcReg operands to DstReg.
// Also update DstReg operands to include DstIdx if it is set.
@@ -3982,17 +3987,18 @@ 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;
diff --git a/llvm/test/CodeGen/LoongArch/register-coalescer-crash-pr79718.mir b/llvm/test/CodeGen/LoongArch/register-coalescer-crash-pr79718.mir
new file mode 100644
index 000000000000000..5e0be0df636b8bf
--- /dev/null
+++ b/llvm/test/CodeGen/LoongArch/register-coalescer-crash-pr79718.mir
@@ -0,0 +1,402 @@
+# 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
+
+--- |
+ source_filename = "register-coalescer-crash-pr79718.ll"
+ target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n64-S128"
+ target triple = "loongarch64"
+
+ define void @foo(ptr nocapture writeonly %0, i1 %1, i1 %2, i1 %3, i1 %4) local_unnamed_addr #0 {
+ start:
+ br label %bb1
+ bb1: ; preds = %bb6, %start
+ %5 = phi i64 [ 0, %start ], [ %20, %bb6 ]
+ %6 = phi i64 [ 0, %start ], [ %21, %bb6 ]
+ %7 = phi i64 [ undef, %start ], [ %23, %bb6 ]
+ br label %bb2
+
+ bb2: ; preds = %bb6, %bb1
+ %8 = phi i64 [ %5, %bb1 ], [ %20, %bb6 ]
+ %9 = phi i64 [ %6, %bb1 ], [ %21, %bb6 ]
+ %10 = phi i64 [ %6, %bb1 ], [ %22, %bb6 ]
+ %11 = phi i64 [ %7, %bb1 ], [ %23, %bb6 ]
+ br i1 %1, label %loopexit, label %preheader.preheader
+
+ preheader.preheader: ; preds = %bb2
+ br label %preheader
+
+ preheader: ; preds = %bb4, %preheader.preheader
+ %12 = phi i64 [ %14, %bb4 ], [ %10, %preheader.preheader ]
+ %13 = load volatile { i64, i64 }, ptr null, align 4294967296
+ br i1 %2, label %bb3, label %bb5
+
+ bb3: ; preds = %preheader
+ br i1 %3, label %bb4, label %bb3.bb5_crit_edge
+
+ bb4: ; preds = %bb3
+ %14 = add i64 %12, 1
+ br i1 %4, label %loopexit, label %preheader
+
+ loopexit: ; preds = %bb4, %bb2
+ %15 = phi i64 [ %9, %bb2 ], [ %14, %bb4 ]
+ %16 = phi i64 [ %10, %bb2 ], [ %14, %bb4 ]
+ store i8 0, ptr %0, align 1
+ br label %bb6
+
+ bb3.bb5_crit_edge: ; preds = %bb3
+ %17 = add i64 %12, 1
+ br label %bb5
+
+ bb5: ; preds = %preheader, %bb3.bb5_crit_edge
+ %18 = phi i64 [ 1, %bb3.bb5_crit_edge ], [ 0, %preheader ]
+ %19 = phi i64 [ %17, %bb3.bb5_crit_edge ], [ %11, %preheader ]
+ store i64 0, ptr %0, align 8
+ br label %bb6
+
+ bb6: ; preds = %bb5, %loopexit
+ %20 = phi i64 [ %19, %bb5 ], [ %8, %loopexit ]
+ %21 = phi i64 [ %18, %bb5 ], [ %15, %loopexit ]
+ %22 = phi i64 [ %18, %bb5 ], [ %16, %loopexit ]
+ %23 = phi i64 [ %19, %bb5 ], [ %11, %loopexit ]
+ %24 = icmp eq i64 %5, 1
+ br i1 %24, label %bb2, label %bb1
+ }
+
+ attributes #0 = { nofree norecurse noreturn nounwind }
+
+...
+---
+name: foo
+alignment: 32
+exposesReturnsTwice: false
+legalized: false
+regBankSelected: false
+selected: false
+failedISel: false
+tracksRegLiveness: true
+hasWinCFI: false
+callsEHReturn: false
+callsUnwindInit: false
+hasEHCatchret: false
+hasEHScopes: false
+hasEHFunclets: false
+isOutlined: false
+debugInstrRef: false
+failsVerification: false
+tracksDebugUserValues: false
+registers:
+ - { id: 0, class: gpr, preferred-register: '' }
+ - { id: 1, class: gpr, preferred-register: '' }
+ - { id: 2, class: gpr, preferred-register: '' }
+ - { id: 3, class: gpr, preferred-register: '' }
+ - { id: 4, class: gpr, preferred-register: '' }
+ - { id: 5, class: gpr, preferred-register: '' }
+ - { id: 6, class: gpr, preferred-register: '' }
+ - { id: 7, class: gpr, preferred-register: '' }
+ - { id: 8, class: gpr, preferred-register: '' }
+ - { id: 9, class: gpr, preferred-register: '' }
+ - { id: 10, class: gpr, preferred-register: '' }
+ - { id: 11, class: gpr, preferred-register: '' }
+ - { id: 12, class: gpr, preferred-register: '' }
+ - { id: 13, class: gpr, preferred-register: '' }
+ - { id: 14, class: gpr, preferred-register: '' }
+ - { id: 15, class: gpr, preferred-register: '' }
+ - { id: 16, class: gpr, preferred-register: '' }
+ - { id: 17, class: gpr, preferred-register: '' }
+ - { id: 18, class: gpr, preferred-register: '' }
+ - { id: 19, class: gpr, preferred-register: '' }
+ - { id: 20, class: gpr, preferred-register: '' }
+ - { id: 21, class: gpr, preferred-register: '' }
+ - { id: 22, class: gpr, preferred-register: '' }
+ - { id: 23, class: gpr, preferred-register: '' }
+ - { id: 24, class: gpr, preferred-register: '' }
+ - { id: 25, class: gpr, preferred-register: '' }
+ - { id: 26, class: gpr, preferred-register: '' }
+ - { id: 27, class: gpr, preferred-register: '' }
+ - { id: 28, class: gpr, preferred-register: '' }
+ - { id: 29, class: gpr, preferred-register: '' }
+ - { id: 30, class: gpr, preferred-register: '' }
+ - { id: 31, class: gpr, preferred-register: '' }
+ - { id: 32, class: gpr, preferred-register: '' }
+ - { id: 33, class: gpr, preferred-register: '' }
+ - { id: 34, class: gpr, preferred-register: '' }
+ - { id: 35, class: gpr, preferred-register: '' }
+ - { id: 36, class: gpr, preferred-register: '' }
+ - { id: 37, class: gpr, preferred-register: '' }
+ - { id: 38, class: gpr, preferred-register: '' }
+ - { id: 39, class: gpr, preferred-register: '' }
+ - { id: 40, class: gpr, preferred-register: '' }
+ - { id: 41, class: gpr, preferred-register: '' }
+ - { id: 42, class: gpr, preferred-register: '' }
+ - { id: 43, class: gpr, preferred-register: '' }
+ - { id: 44, class: gpr, preferred-register: '' }
+ - { id: 45, class: gpr, preferred-register: '' }
+ - { id: 46, class: gpr, preferred-register: '' }
+ - { id: 47, class: gpr, preferred-register: '' }
+ - { id: 48, class: gpr, preferred-register: '' }
+ - { id: 49, class: gpr, preferred-register: '' }
+ - { id: 50, class: gpr, preferred-register: '' }
+ - { id: 51, class: gpr, preferred-register: '' }
+ - { id: 52, class: gpr, preferred-register: '' }
+ - { id: 53, class: gpr, preferred-register: '' }
+ - { id: 54, class: gpr, preferred-register: '' }
+ - { id: 55, class: gpr, preferred-register: '' }
+ - { id: 56, class: gpr, preferred-register: '' }
+ - { id: 57, class: gpr, preferred-register: '' }
+ - { id: 58, class: gpr, preferred-register: '' }
+liveins:
+ - { reg: '$r4', virtual-reg: '%18' }
+ - { reg: '$r5', virtual-reg: '%19' }
+ - { reg: '$r6', virtual-reg: '%20' }
+ - { reg: '$r7', virtual-reg: '%21' }
+ - { reg: '$r8', virtual-reg: '%22' }
+frameInfo:
+ isFrameAddressTaken: false
+ isReturnAddressTaken: false
+ hasStackMap: false
+ hasPatchPoint: false
+ stackSize: 0
+ offsetAdjustment: 0
+ maxAlignment: 1
+ adjustsStack: false
+ hasCalls: false
+ stackProtector: ''
+ functionContext: ''
+ maxCallFrameSize: 4294967295
+ cvBytesOfCalleeSavedRegisters: 0
+ hasOpaqueSPAdjustment: false
+ hasVAStart: false
+ hasMustTailInVarArgFunc: false
+ hasTailCall: false
+ localFrameSize: 0
+ savePoint: ''
+ restorePoint: ''
+fixedStack: []
+stack: []
+entry_values: []
+callSites: []
+debugValueSubstitutions: []
+constants: []
+machineFunctionInfo: {}
+body: |
+ ; CHECK-LABEL: name: foo
+ ; CHECK: bb.0.start:
+ ; 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.bb1:
+ ; CHECK-NEXT: successors: %bb.2(0x80000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: [[COPY7:%[0-9]+]]:gpr = COPY [[COPY5]]
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.2.bb2:
+ ; 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.preheader.preheader:
+ ; CHECK-NEXT: successors: %bb.5(0x80000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.5.preheader:
+ ; CHECK-NEXT: successors: %bb.7(0x7c000000), %bb.6(0x04000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: dead [[LD_D:%[0-9]+]]:gpr = LD_D $r0, 8 :: (volatile load (s64) from `ptr null` + 8, basealign 4294967296)
+ ; CHECK-NEXT: dead [[LD_D1:%[0-9]+]]:gpr = LD_D $r0, 0 :: (volatile load (s64) from `ptr null`, align 4294967296)
+ ; 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.bb3:
+ ; 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.bb4:
+ ; 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.loopexit:
+ ; CHECK-NEXT: successors: %bb.12(0x80000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: ST_B $r0, [[COPY4]], 0 :: (store (s8) into %ir.0)
+ ; CHECK-NEXT: PseudoBR %bb.12
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.10.bb3.bb5_crit_edge:
+ ; 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.bb5:
+ ; CHECK-NEXT: successors: %bb.12(0x80000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: ST_D $r0, [[COPY4]], 0 :: (store (s64) into %ir.0)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.12.bb6:
+ ; 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.start:
+ successors: %bb.1(0x80000000)
+ liveins: $r4, $r5, $r6, $r7, $r8
+
+ %22:gpr = COPY killed $r8
+ %21:gpr = COPY killed $r7
+ %20:gpr = COPY killed $r6
+ %19:gpr = COPY killed $r5
+ %18:gpr = COPY killed $r4
+ %29:gpr = COPY $r0
+ %27:gpr = COPY killed %29
+ %30:gpr = ANDI killed %19, 1
+ %41:gpr = ORI $r0, 1
+ %35:gpr = ANDI killed %20, 1
+ %36:gpr = ANDI killed %21, 1
+ %39:gpr = ANDI killed %22, 1
+ %43:gpr = COPY %27
+ %44:gpr = COPY killed %27
+ %45:gpr = IMPLICIT_DEF
+
+ bb.1.bb1:
+ successors: %bb.2(0x80000000)
+
+ %2:gpr = COPY killed %45
+ %1:gpr = COPY killed %44
+ %0:gpr = COPY killed %43
+ %46:gpr = COPY %0
+ %47:gpr = COPY %1
+ %48:gpr = COPY killed %1
+ %49:gpr = COPY killed %2
+
+ bb.2.bb2:
+ successors: %bb.12(0x40000000), %bb.3(0x40000000)
+
+ %6:gpr = COPY killed %49
+ %5:gpr = COPY killed %48
+ %4:gpr = COPY killed %47
+ %3:gpr = COPY killed %46
+ BEQZ %30, %bb.3
+
+ bb.12:
+ successors: %bb.8(0x80000000)
+
+ %51:gpr = COPY killed %4
+ %52:gpr = COPY killed %5
+ PseudoBR %bb.8
+
+ bb.3.preheader.preheader:
+ successors: %bb.4(0x80000000)
+
+ %50:gpr = COPY killed %5
+
+ bb.4.preheader:
+ successors: %bb.6(0x7c000000), %bb.5(0x04000000)
+
+ %7:gpr = COPY killed %50
+ dead %33:gpr = LD_D $r0, 8 :: (volatile load (s64) from `ptr null` + 8, basealign 4294967296)
+ dead %34:gpr = LD_D $r0, 0 :: (volatile load (s64) from `ptr null`, align 4294967296)
+ BNEZ %35, %bb.6
+
+ bb.5:
+ successors: %bb.10(0x80000000)
+
+ %32:gpr = COPY $r0
+ %31:gpr = COPY killed %32
+ %53:gpr = COPY killed %31
+ %54:gpr = COPY killed %6
+ PseudoBR %bb.10
+
+ bb.6.bb3:
+ successors: %bb.7(0x7c000000), %bb.9(0x04000000)
+
+ BEQZ %36, %bb.9
+ PseudoBR %bb.7
+
+ bb.7.bb4:
+ successors: %bb.8(0x04000000), %bb.4(0x7c000000)
+
+ %8:gpr = ADDI_D killed %7, 1
+ %50:gpr = COPY %8
+ %51:gpr = COPY %8
+ %52:gpr = COPY killed %8
+ BEQZ %39, %bb.4
+ PseudoBR %bb.8
+
+ bb.8.loopexit:
+ successors: %bb.11(0x80000000)
+
+ %10:gpr = COPY killed %52
+ %9:gpr = COPY killed %51
+ %40:gpr = COPY $r0
+ ST_B killed %40, %18, 0 :: (store (s8) into %ir.0)
+ %55:gpr = COPY killed %3
+ %56:gpr = COPY killed %9
+ %57:gpr = COPY killed %10
+ %58:gpr = COPY killed %6
+ PseudoBR %bb.11
+
+ bb.9.bb3.bb5_crit_edge:
+ successors: %bb.10(0x80000000)
+
+ %42:gpr = ADDI_D killed %7, 1
+ %53:gpr = COPY %41
+ %54:gpr = COPY killed %42
+
+ bb.10.bb5:
+ successors: %bb.11(0x80000000)
+
+ %13:gpr = COPY killed %54
+ %12:gpr = COPY killed %53
+ %38:gpr = COPY $r0
+ ST_D killed %38, %18, 0 :: (store (s64) into %ir.0)
+ %55:gpr = COPY %13
+ %56:gpr = COPY %12
+ %57:gpr = COPY killed %12
+ %58:gpr = COPY killed %13
+
+ bb.11.bb6:
+ successors: %bb.2(0x7c000000), %bb.1(0x04000000)
+
+ %17:gpr = COPY killed %58
+ %16:gpr = COPY killed %57
+ %15:gpr = COPY killed %56
+ %14:gpr = COPY killed %55
+ %43:gpr = COPY %14
+ %44:gpr = COPY %15
+ %45:gpr = COPY %17
+ %46:gpr = COPY killed %14
+ %47:gpr = COPY killed %15
+ %48:gpr = COPY killed %16
+ %49:gpr = COPY killed %17
+ BEQ %0, %41, %bb.2
+ PseudoBR %bb.1
+
+...
|
qcolombet
reviewed
Jan 29, 2024
| ErasedInstrs.erase(CopyMI); | ||
| if (ErasedInstrs.erase(CopyMI)) | ||
| // But we may encounter the instruction again in this iteration. | ||
| CurrentErasedInstrs.insert(CopyMI); |
There was a problem hiding this comment.
How do we end up with the same copy instruction twice in the worklist?
There was a problem hiding this comment.
From the debugging results, it looks like removePartialRedundancy changes the contents of the work list.
|
#73519 is potentially another attempt at solving the same issue |
It does seem to be the same issue. I'm not sure which is the better way to handle this. Do you have any more suggestions? (This PR seems to be missing multiple duplicates?) |
|
Ping~ Though I know very little about this part. But I think this at least fixes the crash, with no other regressions. |
|
@qcolombet @arsenm |
DianQK
force-pushed
the
register-coalescer
branch
from
9cb9ad2
to
64ab4f4
Compare
…s until the end of an iteration
DianQK
force-pushed
the
register-coalescer
branch
from
64ab4f4
to
00254d3
Compare
arsenm
reviewed
Feb 8, 2024
llvm/test/CodeGen/LoongArch/register-coalescer-crash-pr79718.mir
Outdated
Show resolved
Hide resolved
arsenm
reviewed
Feb 9, 2024
arsenm
reviewed
Feb 9, 2024
| Progress |= Success; | ||
| if (Success || !Again) | ||
| MI = nullptr; | ||
| } | ||
| // Clear instructions not recorded in `ErasedInstrs` but erased. |
There was a problem hiding this comment.
This whole thing feels overcomplicated but I don't have any better suggestion
There was a problem hiding this comment.
I think all because we are dancing with dangling pointers.
🔩🔩🔩
🔩💃🔩
🔩🔩🔩
I found out why. It's a very specific case. |
…Instrs` but erased.
DianQK
changed the title
ErasedInstrs until the end of an iterationErasedInstrs but erased.
DianQK
changed the title
ErasedInstrs but erased.ErasedInstrs but erased
arsenm
approved these changes
Feb 9, 2024
|
Thanks for your patience in reviewing. |
|
Oops, since #80147 I should rebase. |
Fixed. |
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Feb 9, 2024
… but erased (llvm#79820) Fixes llvm#79718. Fixes llvm#71178. The same instructions may exist in an iteration. We cannot immediately delete instructions in `ErasedInstrs`. (cherry picked from commit 95b14da)
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Feb 9, 2024
… but erased (llvm#79820) Fixes llvm#79718. Fixes llvm#71178. The same instructions may exist in an iteration. We cannot immediately delete instructions in `ErasedInstrs`. (cherry picked from commit 95b14da)
qcolombet
reviewed
Feb 14, 2024
There was a problem hiding this comment.
Sorry guys I'm late to the party.
Finally got time to look closer.
Ultimately I think the issue is we try to maintain a global state whereas the local and non-local worklist are not shared.
In this patch, I feel we are trying to reconstruct which list has been effectively modified and update the other one accordingly.
All in all, I think the complexity is not worth it.
Instead I would recommend that we stop being smart with ErasedInstrs and just remove that call to remove.
If the memory consumption is indeed a problem with ErasedInstrs growing, we can revisit.
I agree. |
tstellar
pushed a commit
to tstellar/llvm-project
that referenced
this pull request
Feb 14, 2024
… but erased (llvm#79820) Fixes llvm#79718. Fixes llvm#71178. The same instructions may exist in an iteration. We cannot immediately delete instructions in `ErasedInstrs`. (cherry picked from commit 95b14da)
tstellar
pushed a commit
to tstellar/llvm-project
that referenced
this pull request
Feb 14, 2024
… but erased (llvm#79820) Fixes llvm#79718. Fixes llvm#71178. The same instructions may exist in an iteration. We cannot immediately delete instructions in `ErasedInstrs`. (cherry picked from commit 95b14da)
tstellar
pushed a commit
to tstellar/llvm-project
that referenced
this pull request
Feb 14, 2024
… but erased (llvm#79820) Fixes llvm#79718. Fixes llvm#71178. The same instructions may exist in an iteration. We cannot immediately delete instructions in `ErasedInstrs`. (cherry picked from commit 95b14da)
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #79718. Fixes #71178.
The same instructions may exist in an iteration. We cannot immediately delete instructions in
ErasedInstrs.Here's my debug logs, you can see that the instructions that are about to be processed have appeared in the work list ahead of time.
This change is probably due to
removePartialRedundancy.Address sanitizer logs