Reorder stack spills so that constants come later. #130329

khuey · 2024-09-13T22:47:03Z

Currently constants are "pulled forward" and have their stack spills emitted first. This confuses LLVM as to where to place breakpoints at function entry, and results in argument values being wrong in the debugger. It's straightforward to avoid emitting the stack spills for constants until arguments/etc have been introduced in debug_introduce_locals, so do that.

Example LLVM IR (irrelevant IR elided):
Before:

define internal void @_ZN11rust_1289457binding17h2c78f956ba4bd2c3E(i64 %a, i64 %b, double %c) unnamed_addr #0 !dbg !178 { start:
  %c.dbg.spill = alloca [8 x i8], align 8
  %b.dbg.spill = alloca [8 x i8], align 8
  %a.dbg.spill = alloca [8 x i8], align 8
  %x.dbg.spill = alloca [4 x i8], align 4
  store i32 0, ptr %x.dbg.spill, align 4, !dbg !192            ; LLVM places breakpoint here.
    #dbg_declare(ptr %x.dbg.spill, !190, !DIExpression(), !192)
  store i64 %a, ptr %a.dbg.spill, align 8
    #dbg_declare(ptr %a.dbg.spill, !187, !DIExpression(), !193)
  store i64 %b, ptr %b.dbg.spill, align 8
    #dbg_declare(ptr %b.dbg.spill, !188, !DIExpression(), !194)
  store double %c, ptr %c.dbg.spill, align 8
    #dbg_declare(ptr %c.dbg.spill, !189, !DIExpression(), !195)
  ret void, !dbg !196
}

After:

define internal void @_ZN11rust_1289457binding17h2c78f956ba4bd2c3E(i64 %a, i64 %b, double %c) unnamed_addr #0 !dbg !178 { start:
  %x.dbg.spill = alloca [4 x i8], align 4
  %c.dbg.spill = alloca [8 x i8], align 8
  %b.dbg.spill = alloca [8 x i8], align 8
  %a.dbg.spill = alloca [8 x i8], align 8
  store i64 %a, ptr %a.dbg.spill, align 8
    #dbg_declare(ptr %a.dbg.spill, !187, !DIExpression(), !192)
  store i64 %b, ptr %b.dbg.spill, align 8
    #dbg_declare(ptr %b.dbg.spill, !188, !DIExpression(), !193)
  store double %c, ptr %c.dbg.spill, align 8
    #dbg_declare(ptr %c.dbg.spill, !189, !DIExpression(), !194)
  store i32 0, ptr %x.dbg.spill, align 4, !dbg !195            ; LLVM places breakpoint here.
    #dbg_declare(ptr %x.dbg.spill, !190, !DIExpression(), !195)
  ret void, !dbg !196
}

Note in particular the position of the "LLVM places breakpoint here" comment relative to the stack spills for the function arguments. LLVM assumes that the first instruction with with a debug location is the end of the prologue. As LLVM does not currently offer front ends any direct control over the placement of the prologue end reordering the IR is the only mechanism available to fix argument values at function entry in the presence of MIR optimizations like SingleUseConsts. Fixes #128945

r? @michaelwoerister

bors · 2024-09-17T20:30:03Z

☔ The latest upstream changes (presumably #130473) made this pull request unmergeable. Please resolve the merge conflicts.

Currently constants are "pulled forward" and have their stack spills emitted first. This confuses LLVM as to where to place breakpoints at function entry, and results in argument values being wrong in the debugger. It's straightforward to avoid emitting the stack spills for constants until arguments/etc have been introduced in debug_introduce_locals, so do that. Example LLVM IR (irrelevant IR elided): Before: define internal void @_ZN11rust_1289457binding17h2c78f956ba4bd2c3E(i64 %a, i64 %b, double %c) unnamed_addr #0 !dbg !178 { start: %c.dbg.spill = alloca [8 x i8], align 8 %b.dbg.spill = alloca [8 x i8], align 8 %a.dbg.spill = alloca [8 x i8], align 8 %x.dbg.spill = alloca [4 x i8], align 4 store i32 0, ptr %x.dbg.spill, align 4, !dbg !192 ; LLVM places breakpoint here. #dbg_declare(ptr %x.dbg.spill, !190, !DIExpression(), !192) store i64 %a, ptr %a.dbg.spill, align 8 #dbg_declare(ptr %a.dbg.spill, !187, !DIExpression(), !193) store i64 %b, ptr %b.dbg.spill, align 8 #dbg_declare(ptr %b.dbg.spill, !188, !DIExpression(), !194) store double %c, ptr %c.dbg.spill, align 8 #dbg_declare(ptr %c.dbg.spill, !189, !DIExpression(), !195) ret void, !dbg !196 } After: define internal void @_ZN11rust_1289457binding17h2c78f956ba4bd2c3E(i64 %a, i64 %b, double %c) unnamed_addr #0 !dbg !178 { start: %x.dbg.spill = alloca [4 x i8], align 4 %c.dbg.spill = alloca [8 x i8], align 8 %b.dbg.spill = alloca [8 x i8], align 8 %a.dbg.spill = alloca [8 x i8], align 8 store i64 %a, ptr %a.dbg.spill, align 8 #dbg_declare(ptr %a.dbg.spill, !187, !DIExpression(), !192) store i64 %b, ptr %b.dbg.spill, align 8 #dbg_declare(ptr %b.dbg.spill, !188, !DIExpression(), !193) store double %c, ptr %c.dbg.spill, align 8 #dbg_declare(ptr %c.dbg.spill, !189, !DIExpression(), !194) store i32 0, ptr %x.dbg.spill, align 4, !dbg !195 ; LLVM places breakpoint here. #dbg_declare(ptr %x.dbg.spill, !190, !DIExpression(), !195) ret void, !dbg !196 } Note in particular the position of the "LLVM places breakpoint here" comment relative to the stack spills for the function arguments. LLVM assumes that the first instruction with with a debug location is the end of the prologue. As LLVM does not currently offer front ends any direct control over the placement of the prologue end reordering the IR is the only mechanism available to fix argument values at function entry in the presence of MIR optimizations like SingleUseConsts. Fixes rust-lang#128945

michaelwoerister · 2024-09-18T09:24:36Z

Thanks for the PR, @khuey! This looks reasonable to me but it would be good if someone with MIR expertise signed off on it.

r? mir

khuey · 2024-09-23T20:10:38Z

Let's try someone who seems to be more active.

r? @davidtwco

davidtwco · 2024-09-25T08:08:05Z

@bors r+

bors · 2024-09-25T08:08:08Z

📌 Commit 652b502 has been approved by davidtwco

It is now in the queue for this repository.

…avidtwco Reorder stack spills so that constants come later. Currently constants are "pulled forward" and have their stack spills emitted first. This confuses LLVM as to where to place breakpoints at function entry, and results in argument values being wrong in the debugger. It's straightforward to avoid emitting the stack spills for constants until arguments/etc have been introduced in debug_introduce_locals, so do that. Example LLVM IR (irrelevant IR elided): Before: ``` define internal void `@_ZN11rust_1289457binding17h2c78f956ba4bd2c3E(i64` %a, i64 %b, double %c) unnamed_addr #0 !dbg !178 { start: %c.dbg.spill = alloca [8 x i8], align 8 %b.dbg.spill = alloca [8 x i8], align 8 %a.dbg.spill = alloca [8 x i8], align 8 %x.dbg.spill = alloca [4 x i8], align 4 store i32 0, ptr %x.dbg.spill, align 4, !dbg !192 ; LLVM places breakpoint here. #dbg_declare(ptr %x.dbg.spill, !190, !DIExpression(), !192) store i64 %a, ptr %a.dbg.spill, align 8 #dbg_declare(ptr %a.dbg.spill, !187, !DIExpression(), !193) store i64 %b, ptr %b.dbg.spill, align 8 #dbg_declare(ptr %b.dbg.spill, !188, !DIExpression(), !194) store double %c, ptr %c.dbg.spill, align 8 #dbg_declare(ptr %c.dbg.spill, !189, !DIExpression(), !195) ret void, !dbg !196 } ``` After: ``` define internal void `@_ZN11rust_1289457binding17h2c78f956ba4bd2c3E(i64` %a, i64 %b, double %c) unnamed_addr #0 !dbg !178 { start: %x.dbg.spill = alloca [4 x i8], align 4 %c.dbg.spill = alloca [8 x i8], align 8 %b.dbg.spill = alloca [8 x i8], align 8 %a.dbg.spill = alloca [8 x i8], align 8 store i64 %a, ptr %a.dbg.spill, align 8 #dbg_declare(ptr %a.dbg.spill, !187, !DIExpression(), !192) store i64 %b, ptr %b.dbg.spill, align 8 #dbg_declare(ptr %b.dbg.spill, !188, !DIExpression(), !193) store double %c, ptr %c.dbg.spill, align 8 #dbg_declare(ptr %c.dbg.spill, !189, !DIExpression(), !194) store i32 0, ptr %x.dbg.spill, align 4, !dbg !195 ; LLVM places breakpoint here. #dbg_declare(ptr %x.dbg.spill, !190, !DIExpression(), !195) ret void, !dbg !196 } ``` Note in particular the position of the "LLVM places breakpoint here" comment relative to the stack spills for the function arguments. LLVM assumes that the first instruction with with a debug location is the end of the prologue. As LLVM does not currently offer front ends any direct control over the placement of the prologue end reordering the IR is the only mechanism available to fix argument values at function entry in the presence of MIR optimizations like SingleUseConsts. Fixes rust-lang#128945 r? `@michaelwoerister`

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#129687 (Implement RFC3137 trim-paths sysroot changes - take 2) - rust-lang#129759 (Stabilize `const_refs_to_static`) - rust-lang#130329 (Reorder stack spills so that constants come later.) - rust-lang#130845 (Utf8Chunks: add link to Utf8Chunk) - rust-lang#130846 (Revert Break into the debugger on panic (129019)) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-09-26T02:37:55Z

⌛ Testing commit 652b502 with merge 76ed7a1...

bors · 2024-09-26T04:56:33Z

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 76ed7a1 to master...

rust-timer · 2024-09-26T06:13:22Z

Finished benchmarking commit (76ed7a1): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	1

Max RSS (memory usage)

Results (secondary 2.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (secondary -0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	3
All ❌✅ (primary)	-	-	0

Bootstrap: 773.661s -> 772.716s (-0.12%)
Artifact size: 340.89 MiB -> 340.95 MiB (0.02%)

rustbot assigned michaelwoerister Sep 13, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 13, 2024

khuey force-pushed the reorder-constant-spills branch from 23c8aa3 to 652b502 Compare September 17, 2024 23:58

rustbot assigned matthewjasper and unassigned michaelwoerister Sep 18, 2024

rustbot assigned davidtwco and unassigned matthewjasper Sep 23, 2024

davidtwco approved these changes Sep 25, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2024

matthiaskrgr mentioned this pull request Sep 25, 2024

Rollup of 5 pull requests #130854

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 26, 2024

bors merged commit 76ed7a1 into rust-lang:master Sep 26, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Sep 26, 2024

khuey mentioned this pull request Sep 26, 2024

tests/debuginfo/macro-stepping.rs remains broken with SingleUseConsts #130896

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorder stack spills so that constants come later. #130329

Reorder stack spills so that constants come later. #130329

khuey commented Sep 13, 2024 •

edited

Loading

bors commented Sep 17, 2024

michaelwoerister commented Sep 18, 2024

khuey commented Sep 23, 2024

davidtwco commented Sep 25, 2024

bors commented Sep 25, 2024

bors commented Sep 26, 2024

bors commented Sep 26, 2024

rust-timer commented Sep 26, 2024

Reorder stack spills so that constants come later. #130329

Reorder stack spills so that constants come later. #130329

Conversation

khuey commented Sep 13, 2024 • edited Loading

bors commented Sep 17, 2024

michaelwoerister commented Sep 18, 2024

khuey commented Sep 23, 2024

davidtwco commented Sep 25, 2024

bors commented Sep 25, 2024

bors commented Sep 26, 2024

bors commented Sep 26, 2024

rust-timer commented Sep 26, 2024

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

khuey commented Sep 13, 2024 •

edited

Loading