[CodeGen][RAGreedy] Inform LiveDebugVariables about snippets spilled by InlineSpiller. #109962

bevin-hansson · 2024-09-25T11:09:29Z

RAGreedy invokes InlineSpiller to spill a particular virtreg inline. When the
spiller does this, it also identifies small, adjacent liveranges called
snippets. These are also spilled or rematerialized in the process.

However, the spiller does not inform RA that it has spilled these regs.
This means that debug variable locations referencing these regs/ranges are
lost.

Mark any spilled regs which do not have a stack slot assigned to them as
allocated to the slot being spilled to to tell LDV that those regs are
located in that slot, even though the regs might no longer exist in the
program after regalloc is finished. Also, inform RA about all of the regs
which were replaced (spilled or rematted), not just the one that was
requested so that it can properly manage the ranges of the debug vars.

…by InlineSpiller. RAGreedy invokes InlineSpiller to spill a particular virtreg inline. When the spiller does this, it also identifies small, adjacent liveranges called snippets. These are also spilled or rematerialized in the process. However, the spiller does not inform RA that it has spilled these regs. This means that debug variable locations referencing these regs/ranges are lost. Mark any spilled regs which do not have a stack slot assigned to them as allocated to the slot being spilled to to tell LDV that those regs are located in that slot, even though the regs might no longer exist in the program after regalloc is finished. Also, inform RA about all of the regs which were replaced (spilled or rematted), not just the one that was requested so that it can properly manage the ranges of the debug vars.

llvmbot · 2024-09-25T11:10:05Z

@llvm/pr-subscribers-debuginfo
@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-regalloc

Author: Bevin Hansson (bevin-hansson)

Changes

RAGreedy invokes InlineSpiller to spill a particular virtreg inline. When the
spiller does this, it also identifies small, adjacent liveranges called
snippets. These are also spilled or rematerialized in the process.

However, the spiller does not inform RA that it has spilled these regs.
This means that debug variable locations referencing these regs/ranges are
lost.

Mark any spilled regs which do not have a stack slot assigned to them as
allocated to the slot being spilled to to tell LDV that those regs are
located in that slot, even though the regs might no longer exist in the
program after regalloc is finished. Also, inform RA about all of the regs
which were replaced (spilled or rematted), not just the one that was
requested so that it can properly manage the ranges of the debug vars.

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

4 Files Affected:

(modified) llvm/include/llvm/CodeGen/Spiller.h (+2)
(modified) llvm/lib/CodeGen/InlineSpiller.cpp (+14-1)
(modified) llvm/lib/CodeGen/RegAllocGreedy.cpp (+2-1)
(added) llvm/test/CodeGen/X86/debug-spilled-snippet.ll (+55)

diff --git a/llvm/include/llvm/CodeGen/Spiller.h b/llvm/include/llvm/CodeGen/Spiller.h
index b2f5485eba02e7..0731db533ad839 100644
--- a/llvm/include/llvm/CodeGen/Spiller.h
+++ b/llvm/include/llvm/CodeGen/Spiller.h
@@ -30,6 +30,8 @@ class Spiller {
   /// spill - Spill the LRE.getParent() live interval.
   virtual void spill(LiveRangeEdit &LRE) = 0;
 
+  virtual ArrayRef<Register> getReplacedRegs() = 0;
+
   virtual void postOptimization() {}
 };
 
diff --git a/llvm/lib/CodeGen/InlineSpiller.cpp b/llvm/lib/CodeGen/InlineSpiller.cpp
index 409879a8b49bcc..93027678c0652b 100644
--- a/llvm/lib/CodeGen/InlineSpiller.cpp
+++ b/llvm/lib/CodeGen/InlineSpiller.cpp
@@ -167,6 +167,10 @@ class InlineSpiller : public Spiller {
   // All registers to spill to StackSlot, including the main register.
   SmallVector<Register, 8> RegsToSpill;
 
+  // All registers that were replaced by the spiller. This includes registers
+  // that were rematerialized; rematted regs are removed from RegsToSpill.
+  SmallVector<Register, 8> RegsReplaced;
+
   // All COPY instructions to/from snippets.
   // They are ignored since both operands refer to the same stack slot.
   // For bundled copies, this will only include the first header copy.
@@ -199,6 +203,7 @@ class InlineSpiller : public Spiller {
         HSpiller(Pass, MF, VRM), VRAI(VRAI) {}
 
   void spill(LiveRangeEdit &) override;
+  ArrayRef<Register> getReplacedRegs() override { return RegsReplaced; }
   void postOptimization() override;
 
 private:
@@ -385,6 +390,7 @@ void InlineSpiller::collectRegsToSpill() {
   // Main register always spills.
   RegsToSpill.assign(1, Reg);
   SnippetCopies.clear();
+  RegsReplaced.assign(RegsToSpill);
 
   // Snippets all have the same original, so there can't be any for an original
   // register.
@@ -405,6 +411,8 @@ void InlineSpiller::collectRegsToSpill() {
     LLVM_DEBUG(dbgs() << "\talso spill snippet " << SnipLI << '\n');
     ++NumSnippets;
   }
+
+  RegsReplaced.assign(RegsToSpill);
 }
 
 bool InlineSpiller::isSibling(Register Reg) {
@@ -1255,8 +1263,13 @@ void InlineSpiller::spillAll() {
   LLVM_DEBUG(dbgs() << "Merged spilled regs: " << *StackInt << '\n');
 
   // Spill around uses of all RegsToSpill.
-  for (Register Reg : RegsToSpill)
+  for (Register Reg : RegsToSpill) {
     spillAroundUses(Reg);
+    // Assign all of the spilled registers to the slot so that
+    // LiveDebugVariables knows about these locations later on.
+    if (VRM.getStackSlot(Reg) == VirtRegMap::NO_STACK_SLOT)
+      VRM.assignVirt2StackSlot(Reg, StackSlot);
+  }
 
   // Hoisted spills may cause dead code.
   if (!DeadDefs.empty()) {
diff --git a/llvm/lib/CodeGen/RegAllocGreedy.cpp b/llvm/lib/CodeGen/RegAllocGreedy.cpp
index 1ad70c86d68e3d..60d1c7e53316c9 100644
--- a/llvm/lib/CodeGen/RegAllocGreedy.cpp
+++ b/llvm/lib/CodeGen/RegAllocGreedy.cpp
@@ -2504,7 +2504,8 @@ MCRegister RAGreedy::selectOrSplitImpl(const LiveInterval &VirtReg,
     // Tell LiveDebugVariables about the new ranges. Ranges not being covered by
     // the new regs are kept in LDV (still mapping to the old register), until
     // we rewrite spilled locations in LDV at a later stage.
-    DebugVars->splitRegister(VirtReg.reg(), LRE.regs(), *LIS);
+    for (Register r : spiller().getReplacedRegs())
+      DebugVars->splitRegister(r, LRE.regs(), *LIS);
 
     if (VerifyEnabled)
       MF->verify(this, "After spilling", &errs());
diff --git a/llvm/test/CodeGen/X86/debug-spilled-snippet.ll b/llvm/test/CodeGen/X86/debug-spilled-snippet.ll
new file mode 100644
index 00000000000000..c05e93f5b15d65
--- /dev/null
+++ b/llvm/test/CodeGen/X86/debug-spilled-snippet.ll
@@ -0,0 +1,55 @@
+; RUN: llc -O3 -mtriple i386 %s -stop-after=livedebugvalues -o - | FileCheck %s
+
+; There should be multiple debug values for this variable after regalloc. The
+; value has been spilled, but we shouldn't lose track of the location because
+; of this.
+
+; CHECK-COUNT-4: DBG_VALUE $ebp, 0, !6, !DIExpression(DW_OP_constu, 16, DW_OP_minus), debug-location !10
+
+define void @main(i32 %call, i32 %xor.i, i1 %tobool4.not, i32 %.pre) #0 !dbg !4 {
+entry:
+  %tobool1.not = icmp ne i32 %call, 0
+  %spec.select = zext i1 %tobool1.not to i32
+  br label %for.body5
+
+for.cond.loopexit.loopexit:                       ; preds = %for.body5
+    #dbg_value(i32 %spec.select, !6, !DIExpression(), !10)
+  %tobool.not.i53 = icmp eq i32 %spec.select, 0
+  br i1 %tobool.not.i53, label %transparent_crc.exit57, label %if.then.i54
+
+for.body5:                                        ; preds = %for.body5, %entry
+  %0 = phi i32 [ 0, %entry ], [ %xor1.i40.i, %for.body5 ]
+  %xor6.i = xor i32 %.pre, %0
+  %shr7.i = ashr i32 %xor6.i, 1
+  %xor17.i = xor i32 %shr7.i, %call
+  %shr18.i = ashr i32 %xor17.i, 1
+  %xor.i.i = xor i32 %shr18.i, %xor.i
+  %arrayidx.i.i = getelementptr [0 x i32], ptr null, i32 0, i32 %xor.i.i
+  %xor1.i40.i = xor i32 %xor.i.i, %call
+  br i1 %tobool4.not, label %for.cond.loopexit.loopexit, label %for.body5
+
+if.then.i54:                                      ; preds = %for.cond.loopexit.loopexit
+  store i64 0, ptr null, align 4
+  br label %transparent_crc.exit57
+
+transparent_crc.exit57:                           ; preds = %if.then.i54, %for.cond.loopexit.loopexit
+  ret void
+}
+
+attributes #0 = { "frame-pointer"="all" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 20.0.0git.prerel", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "xx.c", directory: "/path", checksumkind: CSK_MD5, checksum: "c4b2fc62bca9171ad484c91fb78b8842")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 20, type: !5, scopeLine: 20, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!5 = !DISubroutineType(types: !2)
+!6 = !DILocalVariable(name: "flag", arg: 2, scope: !7, file: !1, line: 8, type: !9)
+!7 = distinct !DISubprogram(name: "transparent_crc", scope: !1, file: !1, line: 8, type: !8, scopeLine: 8, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!8 = distinct !DISubroutineType(types: !2)
+!9 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!10 = !DILocation(line: 0, scope: !7, inlinedAt: !11)
+!11 = distinct !DILocation(line: 28, column: 3, scope: !4)

arsenm

Can you also add a MIR test that just runs the allocator? -stress-regalloc can help shrink it

arsenm · 2024-09-25T11:19:42Z

llvm/lib/CodeGen/InlineSpiller.cpp

+  // All registers that were replaced by the spiller. This includes registers
+  // that were rematerialized; rematted regs are removed from RegsToSpill.
+  SmallVector<Register, 8> RegsReplaced;


If this is a superset of RegsToSpill, can you just use RegsToSpill and a supplemental set for the rematerialize or split cases?

I'm not sure what to return out of the spiller if I do this. It's currently an ArrayRef, but if the result is computed, what should get returned?

Return the separate fields?

bevin-hansson · 2024-09-25T11:27:56Z

Can you also add a MIR test that just runs the allocator? -stress-regalloc can help shrink it

I originally had a MIR test, but for some reason I couldn't get it to produce the incorrect output in the before-patch version. I can try again.

bevin-hansson · 2024-09-25T12:15:48Z

I tried reducing the test a bit further with -stress-regalloc, but couldn't really get it to work. It's the same test as the .ll one. Should I remove the .ll?

arsenm

Can probably shrink the MIR test by using -stress-regalloc

Also I meant add the MIR test in addition to the IR test, not as a full replacement

llvm/test/CodeGen/X86/debug-spilled-snippet.mir

arsenm · 2024-09-25T12:25:09Z

llvm/test/CodeGen/X86/debug-spilled-snippet.mir

+  bb.1:
+    successors: %bb.4(0x30000000), %bb.3(0x50000000)
+
+    DBG_VALUE %0, $noreg, !6, !DIExpression(), debug-location !10


Can drop the metadata from this, and then drop the whole IR section

LiveDebugVariables doesn't seem to like it when I do that, it asserts.

Probably should fix that, it should be less difficult to write debug info mir tests

llvm/test/CodeGen/X86/debug-spilled-snippet.mir

arsenm · 2024-09-25T12:25:56Z

llvm/lib/CodeGen/InlineSpiller.cpp

+  // All registers that were replaced by the spiller through some other method,
+  // e.g. rematerialization.
  SmallVector<Register, 8> RegsReplaced;


Maybe could make this one set, if there's another way to tell which were spilled and which were just rematerialized

I guess we could refrain from erasing the regs from the RegsToSpill list and keep track of where the removed regs are located in the list. But I don't know if that's really cleaner.

I started implementing this change, but it really didn't turn out that well.

bevin-hansson · 2024-09-26T13:02:51Z

llvm/lib/CodeGen/InlineSpiller.cpp

@@ -805,6 +812,7 @@ void InlineSpiller::reMaterializeAll() {

    RegsToSpill[ResultPos++] = Reg;
  }
+  RegsReplaced.assign(RegsToSpill.begin() + ResultPos, RegsToSpill.end());


I realize now that this can't be correct. It doesn't swap the regs into this area, it just copies the remaining ones down into the lower half. So, the rematted regs aren't preserved here.

The upper half of RegsToSpill only contains garbage.

arsenm

lgtm with nits

llvm/test/CodeGen/X86/debug-spilled-snippet.ll

llvm/test/CodeGen/X86/debug-spilled-snippet.mir

This doesn't affect the test, but might as well include it.

…by InlineSpiller. (llvm#109962) RAGreedy invokes InlineSpiller to spill a particular virtreg inline. When the spiller does this, it also identifies small, adjacent liveranges called snippets. These are also spilled or rematerialized in the process. However, the spiller does not inform RA that it has spilled these regs. This means that debug variable locations referencing these regs/ranges are lost. Mark any spilled regs which do not have a stack slot assigned to them as allocated to the slot being spilled to to tell LDV that those regs are located in that slot, even though the regs might no longer exist in the program after regalloc is finished. Also, inform RA about all of the regs which were replaced (spilled or rematted), not just the one that was requested so that it can properly manage the ranges of the debug vars.

llvmbot added backend:X86 llvm:regalloc labels Sep 25, 2024

bevin-hansson added the debuginfo label Sep 25, 2024

arsenm requested review from MatzeB, jayfoad, qcolombet, cdevadas and kparzysz September 25, 2024 11:18

arsenm reviewed Sep 25, 2024

View reviewed changes

Separate spilled and rematted regs, and add a MIR test.

ca3088e

arsenm reviewed Sep 25, 2024

View reviewed changes

Simplify MIR test.

e2d2544

bevin-hansson commented Sep 26, 2024

View reviewed changes

Fill RegsReplaced explicitly instead of using assign.

4387650

The upper half of RegsToSpill only contains garbage.

arsenm approved these changes Sep 30, 2024

View reviewed changes

llvm/test/CodeGen/X86/debug-spilled-snippet.ll Outdated Show resolved Hide resolved

llvm/test/CodeGen/X86/debug-spilled-snippet.mir Outdated Show resolved Hide resolved

bevin-hansson added 2 commits October 2, 2024 08:34

Update tests according to comments.

3fee6ca

Include missed stack object changes.

69b4761

This doesn't affect the test, but might as well include it.

arsenm approved these changes Oct 2, 2024

View reviewed changes

bevin-hansson merged commit 1a65d95 into llvm:main Oct 2, 2024
8 checks passed

[CodeGen][RAGreedy] Inform LiveDebugVariables about snippets spilled by InlineSpiller. #109962

[CodeGen][RAGreedy] Inform LiveDebugVariables about snippets spilled by InlineSpiller. #109962

Uh oh!

Conversation

bevin-hansson commented Sep 25, 2024

Uh oh!

llvmbot commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bevin-hansson commented Sep 25, 2024

Uh oh!

bevin-hansson commented Sep 25, 2024

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Sep 25, 2024 •

edited

Loading