[StackColoring] Delete dead stack slots #72633

mohammed-nurulhoque · 2023-11-17T09:58:19Z

deletes slots that have lifetime markers and the lifetime ranges are empty.

github-actions · 2023-11-17T10:00:49Z

✅ With the latest revision this PR passed the C/C++ code formatter.

deletes slots that have lifetime markers and the lifetime ranges are empty.

nikic

This looks right to me, but I'm not very familiar with this pass.

llvm/test/CodeGen/RISCV/dead-stack-slot.ll

nikic · 2023-11-17T14:05:24Z

There is an assertion failure in llvm/test/DebugInfo/COFF/lexicalblock.ll:

MachineFrameInfo.h:530: int64_t llvm::MachineFrameInfo::getObjectOffset(int) const: Assertion `!isDeadObjectIndex(ObjectIdx) && "Getting frame offset for a dead object?"' failed.

llvmbot · 2023-11-27T11:50:21Z

@llvm/pr-subscribers-debuginfo

Author: None (mohammed-nurulhoque)

Changes

deletes slots that have lifetime markers and the lifetime ranges are empty.

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

5 Files Affected:

(modified) llvm/lib/CodeGen/StackColoring.cpp (+28-4)
(modified) llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll (-1)
(modified) llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll (+2-6)
(added) llvm/test/CodeGen/RISCV/dead-stack-slot.ll (+32)
(modified) llvm/test/DebugInfo/COFF/lexicalblock.ll (+2-22)

diff --git a/llvm/lib/CodeGen/StackColoring.cpp b/llvm/lib/CodeGen/StackColoring.cpp
index 3d261688fa8c817..9d23ddc342739e4 100644
--- a/llvm/lib/CodeGen/StackColoring.cpp
+++ b/llvm/lib/CodeGen/StackColoring.cpp
@@ -899,17 +899,34 @@ void StackColoring::remapInstructions(DenseMap<int, int> &SlotRemap) {
   unsigned FixedMemOp = 0;
   unsigned FixedDbg = 0;
 
-  // Remap debug information that refers to stack slots.
-  for (auto &VI : MF->getVariableDbgInfo()) {
-    if (!VI.Var || !VI.inStackSlot())
+  // Remap debug information that refers to stack slots and delete info for
+  // deleted slots.
+  auto &DbgInfo = MF->getVariableDbgInfo();
+  for (auto it = DbgInfo.begin(); it != DbgInfo.end();) {
+    auto &VI = *it;
+    if (!VI.Var || !VI.inStackSlot()) {
+      it++;
       continue;
+    }
     int Slot = VI.getStackSlot();
+
+    // slot was removed
+    if (Slot >= 0 && Intervals[Slot]->empty() &&
+        InterestingSlots.test(Slot) && !ConservativeSlots.test(Slot)) {
+      LLVM_DEBUG(dbgs() << "Removing debug info for ["
+                        << cast<DILocalVariable>(VI.Var)->getName() << "].\n");
+      it = DbgInfo.erase(it);
+      FixedDbg++;
+      continue;
+    }
+    // Slot was remapped
     if (SlotRemap.count(Slot)) {
       LLVM_DEBUG(dbgs() << "Remapping debug info for ["
                         << cast<DILocalVariable>(VI.Var)->getName() << "].\n");
       VI.updateStackSlot(SlotRemap[Slot]);
       FixedDbg++;
     }
+    it++;
   }
 
   // Keep a list of *allocas* which need to be remapped.
@@ -1249,8 +1266,15 @@ bool StackColoring::runOnMachineFunction(MachineFunction &Func) {
 
   // Do not bother looking at empty intervals.
   for (unsigned I = 0; I < NumSlots; ++I) {
-    if (Intervals[SortedSlots[I]]->empty())
+    int Slot = SortedSlots[I];
+    if (Intervals[Slot]->empty()) {
+      if (InterestingSlots.test(Slot) && !ConservativeSlots.test(Slot)) {
+        RemovedSlots += 1;
+        ReducedSize += MFI->getObjectSize(Slot);
+        MFI->RemoveStackObject(Slot);
+      }
       SortedSlots[I] = -1;
+    }
   }
 
   // This is a simple greedy algorithm for merging allocas. First, sort the
diff --git a/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll b/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
index bf66a1ed042d228..1b0a803734ae9f3 100644
--- a/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
+++ b/llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
@@ -347,7 +347,6 @@ 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 ccf89aac2d5408e..a8684fdfe1c5682 100644
--- a/llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll
+++ b/llvm/test/CodeGen/PowerPC/aix64-cc-abi-vaarg.ll
@@ -138,9 +138,7 @@
 ; 64BIT-LABEL:   fixedStack:
 ; 64BIT-DAG:     - { id: 0, type: default, offset: 112, size: 8, alignment: 16, stack-id: default,
 
-; 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:   stack: []
 
 ; 64BIT-LABEL:   body:             |
 ; 64BIT-DAG:     liveins: $x3, $x4, $x5, $x6, $x7, $x8, $x9, $x10
@@ -305,9 +303,7 @@
 ; 64BIT-LABEL:   fixedStack:
 ; 64BIT-DAG:       - { id: 0, type: default, offset: 152, size: 8
 
-; 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:   stack:           []
 
 ; 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
new file mode 100644
index 000000000000000..fdba359edddaf84
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/dead-stack-slot.ll
@@ -0,0 +1,32 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV32I
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV64I
+
+; Remove the lifetime-marked alloca, but not the unmarked one.
+define dso_local signext i32 @f1() nounwind {
+; RV32I-LABEL: f1:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    addi sp, sp, -32
+; RV32I-NEXT:    li a0, 0
+; RV32I-NEXT:    addi sp, sp, 32
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: f1:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    addi sp, sp, -32
+; RV64I-NEXT:    li a0, 0
+; RV64I-NEXT:    addi sp, sp, 32
+; RV64I-NEXT:    ret
+  %1 = alloca [32 x i8], align 4
+  %2 = alloca [32 x i8], align 4
+  %3 = getelementptr inbounds [32 x i8], [32 x i8]* %1, i64 0, i64 0
+  call void @llvm.lifetime.start.p0i8(i64 32, i8* nonnull %3)
+  call void @llvm.lifetime.end.p0i8(i64 32, i8* nonnull %3)
+  ret i32 0
+}
+
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
+declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
+
diff --git a/llvm/test/DebugInfo/COFF/lexicalblock.ll b/llvm/test/DebugInfo/COFF/lexicalblock.ll
index 40dd8f894252c25..3bfae85f6c9bad9 100644
--- a/llvm/test/DebugInfo/COFF/lexicalblock.ll
+++ b/llvm/test/DebugInfo/COFF/lexicalblock.ll
@@ -63,32 +63,12 @@
 ; CHECK: LocalSym {
 ; CHECK:   VarName: localA
 ; CHECK: }
-; CHECK: LocalSym {
-; CHECK:   VarName: localB
-; 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:   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: LocalSym {
+; CHECK:   VarName: localB
 ; CHECK: }
 ; CHECK: ScopeEndSym {
 ; CHECK:   Kind: S_END {{.*}}

mohammed-nurulhoque · 2023-11-27T11:52:33Z

There is an assertion failure in llvm/test/DebugInfo/COFF/lexicalblock.ll:

MachineFrameInfo.h:530: int64_t llvm::MachineFrameInfo::getObjectOffset(int) const: Assertion `!isDeadObjectIndex(ObjectIdx) && "Getting frame offset for a dead object?"' failed.

The issue was VariableDbgInfos still had DebugInfo for the deleted stack slot. Fixed now.

llvm/lib/CodeGen/StackColoring.cpp

nikic

Looks fine to me now, but I'd like someone more familiar with the backend to take a look at this.

arsenm · 2023-12-07T04:56:44Z

llvm/lib/CodeGen/StackColoring.cpp

+    if (Intervals[Slot]->empty()) {
+      if (InterestingSlots.test(Slot) && !ConservativeSlots.test(Slot)) {
+        RemovedSlots += 1;
+        ReducedSize += MFI->getObjectSize(Slot);


The reduced size thing doesn't account for alignment but I guess this is only used for debug printing and was never accurate

frasercrmck

I can't speak to the StackColoring changes, sorry

llvm/test/CodeGen/RISCV/dead-stack-slot.ll

mohammed-nurulhoque · 2023-12-07T09:33:56Z

Could you please commit the code on my behalf?
Mohammed Nurul Hoque
mohammed.nurulhoque@imgtec.com

arsenm · 2023-12-07T09:37:36Z

Could you please commit the code on my behalf? Mohammed Nurul Hoque mohammed.nurulhoque@imgtec.com

Can you fix the formatting error?

mohammed-nurulhoque · 2023-12-13T11:58:52Z

ping

piotrAMD · 2023-12-13T13:16:23Z

The change is causing the assertion for me in "ninja check-llvm" on linux:
llc: /upstream/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.

nikic · 2023-12-13T13:20:20Z

@piotrAMD What's the failing test?

piotrAMD · 2023-12-13T13:24:15Z

@piotrAMD What's the failing test?

LLVM :: DebugInfo/COFF/lexicalblock.ll

Also visible here: https://buildkite.com/llvm-project/github-pull-requests/builds/22410#018c633f-a0f4-4604-8c7d-17deb1708815

This reverts commit a294578. Causes an assertion failure in llvm/test/DebugInfo/COFF/lexicalblock.ll.

nikic · 2023-12-13T13:32:06Z

Thanks, I see it as well. I've reverted the patch. It's weird that this did not fail in the buildkite run on the PR.

ShivaChen requested review from rnk and nikic November 17, 2023 10:19

delete dead stack slots (llvm#104)

24f1b7a

deletes slots that have lifetime markers and the lifetime ranges are empty.

mohammed-nurulhoque force-pushed the ussc branch from df700d9 to 24f1b7a Compare November 17, 2023 10:30

nikic changed the title ~~delete dead stack slots (#104)~~ [StackColoring] Delete dead stack slots Nov 17, 2023

nikic reviewed Nov 17, 2023

View reviewed changes

llvm/test/CodeGen/RISCV/dead-stack-slot.ll Outdated Show resolved Hide resolved

llvmbot added the debuginfo label Nov 27, 2023

nikic reviewed Nov 27, 2023

View reviewed changes

llvm/lib/CodeGen/StackColoring.cpp Outdated Show resolved Hide resolved

mohammed-nurulhoque force-pushed the ussc branch from c6c7753 to d2f103f Compare November 28, 2023 10:04

Fix Debug Info after removing stack slot

cc6ae37

mohammed-nurulhoque force-pushed the ussc branch from d2f103f to cc6ae37 Compare November 28, 2023 10:18

use-opaque-pointer

dda2242

mohammed-nurulhoque requested a review from nikic November 28, 2023 10:25

nikic reviewed Nov 28, 2023

View reviewed changes

llvm/lib/CodeGen/StackColoring.cpp Show resolved Hide resolved

check debuginfo is stack slot

81309b7

bzEq requested review from MatzeB, arsenm and qcolombet November 30, 2023 07:00

nikic reviewed Nov 30, 2023

View reviewed changes

ShivaChen requested a review from frasercrmck December 6, 2023 09:10

arsenm approved these changes Dec 7, 2023

View reviewed changes

frasercrmck reviewed Dec 7, 2023

View reviewed changes

llvm/test/CodeGen/RISCV/dead-stack-slot.ll Outdated Show resolved Hide resolved

simplify test case

80a04b1

fix formatting

71f88f6

nikic merged commit a294578 into llvm:main Dec 13, 2023
4 checks passed

nikic added a commit that referenced this pull request Dec 13, 2023

Revert "[StackColoring] Delete dead stack slots (#72633)"

9c093cb

This reverts commit a294578. Causes an assertion failure in llvm/test/DebugInfo/COFF/lexicalblock.ll.

mohammed-nurulhoque mentioned this pull request Dec 13, 2023

[StackColoring] Delete dead stack slots #75351

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[StackColoring] Delete dead stack slots #72633

[StackColoring] Delete dead stack slots #72633

mohammed-nurulhoque commented Nov 17, 2023

github-actions bot commented Nov 17, 2023 •

edited

nikic left a comment

nikic commented Nov 17, 2023

llvmbot commented Nov 27, 2023

mohammed-nurulhoque commented Nov 27, 2023

nikic left a comment

arsenm Dec 7, 2023

frasercrmck left a comment

mohammed-nurulhoque commented Dec 7, 2023

arsenm commented Dec 7, 2023

mohammed-nurulhoque commented Dec 13, 2023

piotrAMD commented Dec 13, 2023

nikic commented Dec 13, 2023

piotrAMD commented Dec 13, 2023

nikic commented Dec 13, 2023

[StackColoring] Delete dead stack slots #72633

[StackColoring] Delete dead stack slots #72633

Conversation

mohammed-nurulhoque commented Nov 17, 2023

github-actions bot commented Nov 17, 2023 • edited

nikic left a comment

Choose a reason for hiding this comment

nikic commented Nov 17, 2023

llvmbot commented Nov 27, 2023

mohammed-nurulhoque commented Nov 27, 2023

nikic left a comment

Choose a reason for hiding this comment

arsenm Dec 7, 2023

Choose a reason for hiding this comment

frasercrmck left a comment

Choose a reason for hiding this comment

mohammed-nurulhoque commented Dec 7, 2023

arsenm commented Dec 7, 2023

mohammed-nurulhoque commented Dec 13, 2023

piotrAMD commented Dec 13, 2023

nikic commented Dec 13, 2023

piotrAMD commented Dec 13, 2023

nikic commented Dec 13, 2023

github-actions bot commented Nov 17, 2023 •

edited