Skip to content

Revert "[StackColoring] Delete dead stack slots" #75639

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

Closed
wants to merge 1 commit into from

Conversation

ilovepi
Copy link
Contributor

@ilovepi ilovepi commented Dec 15, 2023

Reverts #75351

The patch causes the following assert to trigger:
llvm/include/llvm/CodeGen/MachineFrameInfo.h:530: int64_t llvm::MachineFrameInfo::getObjectOffset(int) const: Assertion `!isDeadObjectIndex(ObjectIdx) && "Getting frame offset for a dead object?"' failed.

@llvmbot
Copy link
Member

llvmbot commented Dec 15, 2023

@llvm/pr-subscribers-debuginfo

Author: Paul Kirth (ilovepi)

Changes

Reverts llvm/llvm-project#75351

The patch causes the following assert to trigger:
llvm/include/llvm/CodeGen/MachineFrameInfo.h:530: int64_t llvm::MachineFrameInfo::getObjectOffset(int) const: Assertion `!isDeadObjectIndex(ObjectIdx) && "Getting frame offset for a dead object?"' failed.


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

6 Files Affected:

  • (modified) llvm/lib/CodeGen/StackColoring.cpp (+2-18)
  • (modified) llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll (+1)
  • (modified) llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll (+6-2)
  • (removed) llvm/test/CodeGen/RISCV/dead-stack-slot.ll (-25)
  • (modified) llvm/test/CodeGen/X86/StackColoring-tbaa.mir (+1-1)
  • (modified) llvm/test/DebugInfo/COFF/lexicalblock.ll (+22-2)
diff --git a/llvm/lib/CodeGen/StackColoring.cpp b/llvm/lib/CodeGen/StackColoring.cpp
index fa01aa17b3a867..37f7aa9290054e 100644
--- a/llvm/lib/CodeGen/StackColoring.cpp
+++ b/llvm/lib/CodeGen/StackColoring.cpp
@@ -900,15 +900,6 @@ void StackColoring::remapInstructions(DenseMap<int, int> &SlotRemap) {
   unsigned FixedMemOp = 0;
   unsigned FixedDbg = 0;
 
-  // Remove debug information for deleted slots.
-  erase_if(MF->getVariableDbgInfo(), [&](auto &VI) {
-    if (!VI.inStackSlot())
-      return false;
-    int Slot = VI.getStackSlot();
-    return Slot >= 0 && Intervals[Slot]->empty() &&
-           InterestingSlots.test(Slot) && !ConservativeSlots.test(Slot);
-  });
-
   // Remap debug information that refers to stack slots.
   for (auto &VI : MF->getVariableDbgInfo()) {
     if (!VI.Var || !VI.inStackSlot())
@@ -1259,15 +1250,8 @@ bool StackColoring::runOnMachineFunction(MachineFunction &Func) {
 
   // Do not bother looking at empty intervals.
   for (unsigned I = 0; I < NumSlots; ++I) {
-    int Slot = SortedSlots[I];
-    if (Intervals[Slot]->empty()) {
-      if (InterestingSlots.test(Slot) && !ConservativeSlots.test(Slot)) {
-        RemovedSlots += 1;
-        ReducedSize += MFI->getObjectSize(Slot);
-        MFI->RemoveStackObject(Slot);
-      }
+    if (Intervals[SortedSlots[I]]->empty())
       SortedSlots[I] = -1;
-    }
   }
 
   // This is a simple greedy algorithm for merging allocas. First, sort the
@@ -1355,7 +1339,7 @@ bool StackColoring::runOnMachineFunction(MachineFunction &Func) {
 
   // Scan the entire function and update all machine operands that use frame
   // indices to use the remapped frame index.
-  if (RemovedSlots > 0) {
+  if (!SlotRemap.empty()) {
     expungeSlotMap(SlotRemap, NumSlots);
     remapInstructions(SlotRemap);
   }
diff --git a/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll b/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
index 1b0a803734ae9f..bf66a1ed042d22 100644
--- a/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
+++ b/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
@@ -347,6 +347,7 @@ entry:
 
 ; 32BIT-LABEL:   stack:
 ; 32BIT-DAG:     - { id: 0, name: arg1, type: default, offset: 0, size: 4, alignment: 4,
+; 32BIT-DAG:     - { id: 1, name: arg2, type: default, offset: 0, size: 4, alignment: 4,
 ; 32BIT-DAG:     - { id: 2, name: '', type: default, offset: 0, size: 8, alignment: 8,
 ; 32BIT-DAG:     - { id: 3, name: '', type: default, offset: 0, size: 8, alignment: 8,
 
diff --git a/llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll b/llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll
index a8684fdfe1c568..ccf89aac2d5408 100644
--- a/llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll
+++ b/llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll
@@ -138,7 +138,9 @@
 ; 64BIT-LABEL:   fixedStack:
 ; 64BIT-DAG:     - { id: 0, type: default, offset: 112, size: 8, alignment: 16, stack-id: default,
 
-; 64BIT-LABEL:   stack: []
+; 64BIT-LABEL:   stack:
+; 64BIT-DAG:     - { id: 0, name: arg1, type: default, offset: 0, size: 8, alignment: 8,
+; 64BIT-DAG:     - { id: 1, name: arg2, type: default, offset: 0, size: 8, alignment: 8,
 
 ; 64BIT-LABEL:   body:             |
 ; 64BIT-DAG:     liveins: $x3, $x4, $x5, $x6, $x7, $x8, $x9, $x10
@@ -303,7 +305,9 @@
 ; 64BIT-LABEL:   fixedStack:
 ; 64BIT-DAG:       - { id: 0, type: default, offset: 152, size: 8
 
-; 64BIT-LABEL:   stack:           []
+; 64BIT-LABEL:   stack:
+; 64BIT-DAG:       - { id: 0, name: arg1, type: default, offset: 0, size: 8
+; 64BIT-DAG:       - { id: 1, name: arg2, type: default, offset: 0, size: 8
 
 ; 64BIT-LABEL:     body:             |
 ; 64BIT-DAG:       liveins: $f1, $f2, $f3, $f4, $f5, $f6, $f7, $f8, $f9, $f10, $f11, $f12, $f13
diff --git a/llvm/test/CodeGen/RISCV/dead-stack-slot.ll b/llvm/test/CodeGen/RISCV/dead-stack-slot.ll
deleted file mode 100644
index 49b0d2ab58c4f6..00000000000000
--- a/llvm/test/CodeGen/RISCV/dead-stack-slot.ll
+++ /dev/null
@@ -1,25 +0,0 @@
-; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
-; RUN:   | FileCheck %s
-; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
-; RUN:   | FileCheck %s
-
-; Remove the lifetime-marked alloca, but not the unmarked one.
-define signext i32 @f1() nounwind {
-; CHECK-LABEL: f1:
-; CHECK:       # %bb.0:
-; CHECK-NEXT:    addi sp, sp, -32
-; CHECK-NEXT:    li a0, 0
-; CHECK-NEXT:    addi sp, sp, 32
-; CHECK-NEXT:    ret
-  %1 = alloca [32 x i8], align 4
-  %2 = alloca [32 x i8], align 4
-  %3 = getelementptr inbounds [32 x i8], ptr %1, i64 0, i64 0
-  call void @llvm.lifetime.start.p0(i64 32, ptr nonnull %3)
-  call void @llvm.lifetime.end.p0(i64 32, ptr nonnull %3)
-  ret i32 0
-}
-
-declare void @llvm.lifetime.start.p0(i64 immarg, ptr nocapture)
-declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture)
-
diff --git a/llvm/test/CodeGen/X86/StackColoring-tbaa.mir b/llvm/test/CodeGen/X86/StackColoring-tbaa.mir
index b4fdf4d2ec9176..6d7f294549d791 100644
--- a/llvm/test/CodeGen/X86/StackColoring-tbaa.mir
+++ b/llvm/test/CodeGen/X86/StackColoring-tbaa.mir
@@ -53,7 +53,7 @@ body:             |
     ; CHECK: [[LEA64r:%[0-9]+]]:gr64 = nuw LEA64r %stack.1.agg, 1, $noreg, 24, $noreg
     ; CHECK-NEXT: CMP8mi %stack.1.agg, 1, $noreg, 47, $noreg, 0, implicit-def $eflags :: (dereferenceable load (s8) from %ir.a22, !tbaa !2)
     ; CHECK-NEXT: [[CMOV64rm:%[0-9]+]]:gr64 = CMOV64rm [[LEA64r]], %stack.1.agg, 1, $noreg, 24, $noreg, 8, implicit $eflags :: (dereferenceable load (s64) from %ir.a2)
-    ; CHECK-NEXT: [[MOV8rm:%[0-9]+]]:gr8 = MOV8rm killed [[CMOV64rm]], 1, $noreg, 16, $noreg :: (load (s8) from %ir.add.ptr.i)
+    ; CHECK-NEXT: [[MOV8rm:%[0-9]+]]:gr8 = MOV8rm killed [[CMOV64rm]], 1, $noreg, 16, $noreg :: (load (s8) from %ir.add.ptr.i, !tbaa !2)
     ; CHECK-NEXT: $al = COPY [[MOV8rm]]
     ; CHECK-NEXT: RET 0, $al
     LIFETIME_START %stack.0.padding
diff --git a/llvm/test/DebugInfo/COFF/lexicalblock.ll b/llvm/test/DebugInfo/COFF/lexicalblock.ll
index 3bfae85f6c9bad..40dd8f894252c2 100644
--- a/llvm/test/DebugInfo/COFF/lexicalblock.ll
+++ b/llvm/test/DebugInfo/COFF/lexicalblock.ll
@@ -63,12 +63,32 @@
 ; CHECK: LocalSym {
 ; CHECK:   VarName: localA
 ; CHECK: }
+; CHECK: LocalSym {
+; CHECK:   VarName: localB
+; CHECK: }
 ; CHECK: BlockSym {
 ; CHECK:   Kind: S_BLOCK32 {{.*}}
 ; CHECK:   BlockName: 
 ; CHECK: }
-; CHECK: LocalSym {
-; CHECK:   VarName: localB
+; CHECK: ScopeEndSym {
+; CHECK:   Kind: S_END {{.*}}
+; CHECK: }
+; CHECK: BlockSym {
+; CHECK:   Kind: S_BLOCK32 {{.*}}
+; CHECK:   BlockName: 
+; CHECK: }
+; CHECK: ScopeEndSym {
+; CHECK:   Kind: S_END {{.*}}
+; CHECK: }
+; CHECK: BlockSym {
+; CHECK:   Kind: S_BLOCK32 {{.*}}
+; CHECK:   BlockName: 
+; CHECK: }
+; CHECK: ScopeEndSym {
+; CHECK: }
+; CHECK: BlockSym {
+; CHECK:   Kind: S_BLOCK32 {{.*}}
+; CHECK:   BlockName: 
 ; CHECK: }
 ; CHECK: ScopeEndSym {
 ; CHECK:   Kind: S_END {{.*}}

@llvmbot
Copy link
Member

llvmbot commented Dec 15, 2023

@llvm/pr-subscribers-backend-x86

Author: Paul Kirth (ilovepi)

Changes

Reverts llvm/llvm-project#75351

The patch causes the following assert to trigger:
llvm/include/llvm/CodeGen/MachineFrameInfo.h:530: int64_t llvm::MachineFrameInfo::getObjectOffset(int) const: Assertion `!isDeadObjectIndex(ObjectIdx) && "Getting frame offset for a dead object?"' failed.


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

6 Files Affected:

  • (modified) llvm/lib/CodeGen/StackColoring.cpp (+2-18)
  • (modified) llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll (+1)
  • (modified) llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll (+6-2)
  • (removed) llvm/test/CodeGen/RISCV/dead-stack-slot.ll (-25)
  • (modified) llvm/test/CodeGen/X86/StackColoring-tbaa.mir (+1-1)
  • (modified) llvm/test/DebugInfo/COFF/lexicalblock.ll (+22-2)
diff --git a/llvm/lib/CodeGen/StackColoring.cpp b/llvm/lib/CodeGen/StackColoring.cpp
index fa01aa17b3a867..37f7aa9290054e 100644
--- a/llvm/lib/CodeGen/StackColoring.cpp
+++ b/llvm/lib/CodeGen/StackColoring.cpp
@@ -900,15 +900,6 @@ void StackColoring::remapInstructions(DenseMap<int, int> &SlotRemap) {
   unsigned FixedMemOp = 0;
   unsigned FixedDbg = 0;
 
-  // Remove debug information for deleted slots.
-  erase_if(MF->getVariableDbgInfo(), [&](auto &VI) {
-    if (!VI.inStackSlot())
-      return false;
-    int Slot = VI.getStackSlot();
-    return Slot >= 0 && Intervals[Slot]->empty() &&
-           InterestingSlots.test(Slot) && !ConservativeSlots.test(Slot);
-  });
-
   // Remap debug information that refers to stack slots.
   for (auto &VI : MF->getVariableDbgInfo()) {
     if (!VI.Var || !VI.inStackSlot())
@@ -1259,15 +1250,8 @@ bool StackColoring::runOnMachineFunction(MachineFunction &Func) {
 
   // Do not bother looking at empty intervals.
   for (unsigned I = 0; I < NumSlots; ++I) {
-    int Slot = SortedSlots[I];
-    if (Intervals[Slot]->empty()) {
-      if (InterestingSlots.test(Slot) && !ConservativeSlots.test(Slot)) {
-        RemovedSlots += 1;
-        ReducedSize += MFI->getObjectSize(Slot);
-        MFI->RemoveStackObject(Slot);
-      }
+    if (Intervals[SortedSlots[I]]->empty())
       SortedSlots[I] = -1;
-    }
   }
 
   // This is a simple greedy algorithm for merging allocas. First, sort the
@@ -1355,7 +1339,7 @@ bool StackColoring::runOnMachineFunction(MachineFunction &Func) {
 
   // Scan the entire function and update all machine operands that use frame
   // indices to use the remapped frame index.
-  if (RemovedSlots > 0) {
+  if (!SlotRemap.empty()) {
     expungeSlotMap(SlotRemap, NumSlots);
     remapInstructions(SlotRemap);
   }
diff --git a/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll b/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
index 1b0a803734ae9f..bf66a1ed042d22 100644
--- a/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
+++ b/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
@@ -347,6 +347,7 @@ entry:
 
 ; 32BIT-LABEL:   stack:
 ; 32BIT-DAG:     - { id: 0, name: arg1, type: default, offset: 0, size: 4, alignment: 4,
+; 32BIT-DAG:     - { id: 1, name: arg2, type: default, offset: 0, size: 4, alignment: 4,
 ; 32BIT-DAG:     - { id: 2, name: '', type: default, offset: 0, size: 8, alignment: 8,
 ; 32BIT-DAG:     - { id: 3, name: '', type: default, offset: 0, size: 8, alignment: 8,
 
diff --git a/llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll b/llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll
index a8684fdfe1c568..ccf89aac2d5408 100644
--- a/llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll
+++ b/llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll
@@ -138,7 +138,9 @@
 ; 64BIT-LABEL:   fixedStack:
 ; 64BIT-DAG:     - { id: 0, type: default, offset: 112, size: 8, alignment: 16, stack-id: default,
 
-; 64BIT-LABEL:   stack: []
+; 64BIT-LABEL:   stack:
+; 64BIT-DAG:     - { id: 0, name: arg1, type: default, offset: 0, size: 8, alignment: 8,
+; 64BIT-DAG:     - { id: 1, name: arg2, type: default, offset: 0, size: 8, alignment: 8,
 
 ; 64BIT-LABEL:   body:             |
 ; 64BIT-DAG:     liveins: $x3, $x4, $x5, $x6, $x7, $x8, $x9, $x10
@@ -303,7 +305,9 @@
 ; 64BIT-LABEL:   fixedStack:
 ; 64BIT-DAG:       - { id: 0, type: default, offset: 152, size: 8
 
-; 64BIT-LABEL:   stack:           []
+; 64BIT-LABEL:   stack:
+; 64BIT-DAG:       - { id: 0, name: arg1, type: default, offset: 0, size: 8
+; 64BIT-DAG:       - { id: 1, name: arg2, type: default, offset: 0, size: 8
 
 ; 64BIT-LABEL:     body:             |
 ; 64BIT-DAG:       liveins: $f1, $f2, $f3, $f4, $f5, $f6, $f7, $f8, $f9, $f10, $f11, $f12, $f13
diff --git a/llvm/test/CodeGen/RISCV/dead-stack-slot.ll b/llvm/test/CodeGen/RISCV/dead-stack-slot.ll
deleted file mode 100644
index 49b0d2ab58c4f6..00000000000000
--- a/llvm/test/CodeGen/RISCV/dead-stack-slot.ll
+++ /dev/null
@@ -1,25 +0,0 @@
-; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
-; RUN:   | FileCheck %s
-; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
-; RUN:   | FileCheck %s
-
-; Remove the lifetime-marked alloca, but not the unmarked one.
-define signext i32 @f1() nounwind {
-; CHECK-LABEL: f1:
-; CHECK:       # %bb.0:
-; CHECK-NEXT:    addi sp, sp, -32
-; CHECK-NEXT:    li a0, 0
-; CHECK-NEXT:    addi sp, sp, 32
-; CHECK-NEXT:    ret
-  %1 = alloca [32 x i8], align 4
-  %2 = alloca [32 x i8], align 4
-  %3 = getelementptr inbounds [32 x i8], ptr %1, i64 0, i64 0
-  call void @llvm.lifetime.start.p0(i64 32, ptr nonnull %3)
-  call void @llvm.lifetime.end.p0(i64 32, ptr nonnull %3)
-  ret i32 0
-}
-
-declare void @llvm.lifetime.start.p0(i64 immarg, ptr nocapture)
-declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture)
-
diff --git a/llvm/test/CodeGen/X86/StackColoring-tbaa.mir b/llvm/test/CodeGen/X86/StackColoring-tbaa.mir
index b4fdf4d2ec9176..6d7f294549d791 100644
--- a/llvm/test/CodeGen/X86/StackColoring-tbaa.mir
+++ b/llvm/test/CodeGen/X86/StackColoring-tbaa.mir
@@ -53,7 +53,7 @@ body:             |
     ; CHECK: [[LEA64r:%[0-9]+]]:gr64 = nuw LEA64r %stack.1.agg, 1, $noreg, 24, $noreg
     ; CHECK-NEXT: CMP8mi %stack.1.agg, 1, $noreg, 47, $noreg, 0, implicit-def $eflags :: (dereferenceable load (s8) from %ir.a22, !tbaa !2)
     ; CHECK-NEXT: [[CMOV64rm:%[0-9]+]]:gr64 = CMOV64rm [[LEA64r]], %stack.1.agg, 1, $noreg, 24, $noreg, 8, implicit $eflags :: (dereferenceable load (s64) from %ir.a2)
-    ; CHECK-NEXT: [[MOV8rm:%[0-9]+]]:gr8 = MOV8rm killed [[CMOV64rm]], 1, $noreg, 16, $noreg :: (load (s8) from %ir.add.ptr.i)
+    ; CHECK-NEXT: [[MOV8rm:%[0-9]+]]:gr8 = MOV8rm killed [[CMOV64rm]], 1, $noreg, 16, $noreg :: (load (s8) from %ir.add.ptr.i, !tbaa !2)
     ; CHECK-NEXT: $al = COPY [[MOV8rm]]
     ; CHECK-NEXT: RET 0, $al
     LIFETIME_START %stack.0.padding
diff --git a/llvm/test/DebugInfo/COFF/lexicalblock.ll b/llvm/test/DebugInfo/COFF/lexicalblock.ll
index 3bfae85f6c9bad..40dd8f894252c2 100644
--- a/llvm/test/DebugInfo/COFF/lexicalblock.ll
+++ b/llvm/test/DebugInfo/COFF/lexicalblock.ll
@@ -63,12 +63,32 @@
 ; CHECK: LocalSym {
 ; CHECK:   VarName: localA
 ; CHECK: }
+; CHECK: LocalSym {
+; CHECK:   VarName: localB
+; CHECK: }
 ; CHECK: BlockSym {
 ; CHECK:   Kind: S_BLOCK32 {{.*}}
 ; CHECK:   BlockName: 
 ; CHECK: }
-; CHECK: LocalSym {
-; CHECK:   VarName: localB
+; CHECK: ScopeEndSym {
+; CHECK:   Kind: S_END {{.*}}
+; CHECK: }
+; CHECK: BlockSym {
+; CHECK:   Kind: S_BLOCK32 {{.*}}
+; CHECK:   BlockName: 
+; CHECK: }
+; CHECK: ScopeEndSym {
+; CHECK:   Kind: S_END {{.*}}
+; CHECK: }
+; CHECK: BlockSym {
+; CHECK:   Kind: S_BLOCK32 {{.*}}
+; CHECK:   BlockName: 
+; CHECK: }
+; CHECK: ScopeEndSym {
+; CHECK: }
+; CHECK: BlockSym {
+; CHECK:   Kind: S_BLOCK32 {{.*}}
+; CHECK:   BlockName: 
 ; CHECK: }
 ; CHECK: ScopeEndSym {
 ; CHECK:   Kind: S_END {{.*}}

@ilovepi
Copy link
Contributor Author

ilovepi commented Dec 15, 2023

ugh, this isn't clean

@ilovepi ilovepi closed this Dec 15, 2023
@nunoplopes nunoplopes deleted the revert-75351-ussc branch December 27, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants