Added llvm lifetime annotations to function call argument temporaries. #98377

davidv1992 · 2022-06-22T09:56:48Z

The goal of this change is to ensure that llvm will do stack slot
optimization on these temporaries. This ensures that in code like:

const A: [u8; 1024] = [0; 1024];

fn copy_const() {
    f(A);
    f(A);
}

we only use 1024 bytes of stack space, instead of 2048 bytes.

I am new to developing for the rust compiler, and as such not entirely sure, but I believe this should be sufficient to close #98156.

Also, this does not contain a test case to ensure this keeps working, primarily because I am not sure how to go about testing this. I would love some suggestions as to how that could be approached.

The goal of this change is to ensure that llvm will do stack slot optimization on these temporaries. This ensures that in code like: ```rust const A: [u8; 1024] = [0; 1024]; fn copy_const() { f(A); f(A); } ``` we only use 1024 bytes of stack space, instead of 2048 bytes.

rust-highfive · 2022-06-22T09:56:52Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

Please see the contribution instructions for more information.

davidtwco

This looks good to me. I think a mir-opt test might be appropriate, as that should observe the addition of the StorageLive and StorageDead statements for the copy.

compiler/rustc_codegen_ssa/src/mir/block.rs

erikdesjardins · 2022-06-24T16:15:01Z

There are no StorageLive/StorageDead for these arguments in MIR, since const arguments don't get stored to locals in MIR. See https://godbolt.org/z/bKfqYfno1. These temporaries only exist in the codegen backend.

(An alternative way to solve this would be to change MIR semantics so that isn't the case, e.g. by removing the ability to have constant arguments, requiring all arguments to be places, and thus making argument copies explicit in MIR. This might allow additional optimizations [perhaps removing unnecessary copies? I haven't really thought about it]. But I think that would be much more complex than this change.)

The following codegen test should work (maybe with some tweaks, haven't tried it locally), e.g. as src/test/codegen/issue-98156-const-arg-temp-lifetime.rs:

// This test checks that temporaries for indirectly-passed arguments get lifetime markers.

// compile-flags: -O -C no-prepopulate-passes -Zmir-opt-level=0

#![crate_type = "lib"]

extern "Rust" {
    fn f(x: [u8; 1024]);
}

const A: [u8; 1024] = [0; 1024];

// CHECK-LABEL: @const_arg_indirect
#[no_mangle]
pub unsafe fn const_arg_indirect() {
    // Ensure that the live ranges for the two argument temporaries don't overlap.

    // CHECK: call void @llvm.lifetime.start
    // CHECK: call void @f
    // CHECK: call void @llvm.lifetime.end
    // CHECK: call void @llvm.lifetime.start
    // CHECK: call void @f
    // CHECK: call void @llvm.lifetime.end

    f(A);
    f(A);
}

davidv1992 · 2022-06-25T14:49:00Z

On the suggestion of changing MIR semantics, that would make the temporaries available for optimization in MIR, but at the cost of a move that might make things more difficult around constant folding. And changing MIR semantics this way would make it necessary for all constants, even those which are small and currently passed by value instead of by reference. I'm not sure whether that trade-off is worth it.

davidv1992 · 2022-06-25T14:49:38Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

davidtwco · 2022-06-28T15:11:15Z

r? @oli-obk

Seems like this needs someone more recently familiar with MIR than me :)

oli-obk · 2022-06-29T09:44:11Z

@bors try @rust-timer queue

rust-timer · 2022-06-29T09:44:13Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-06-29T09:44:20Z

⌛ Trying commit 259a7a7 with merge e63015e2cd9d6ff835d4ea545bf9bac4f90cea49...

oli-obk · 2022-06-29T09:47:12Z

I have played with the thought of storing all constants in locals and thus massively simplifying Operand (by having an Rvalue::Const instead). It would certainly be interesting to experiment with it to see the impact (const eval will likely suffer), but I wouldn't want to do any change here until @JakobDegen was able to try out explicit SSA registers in MIR.

bors · 2022-06-29T11:34:50Z

☀️ Try build successful - checks-actions
Build commit: e63015e2cd9d6ff835d4ea545bf9bac4f90cea49 (e63015e2cd9d6ff835d4ea545bf9bac4f90cea49)

rust-timer · 2022-06-29T11:34:54Z

Queued e63015e2cd9d6ff835d4ea545bf9bac4f90cea49 with parent 493c960, future comparison URL.

rust-timer · 2022-06-29T15:04:38Z

Finished benchmarking commit (e63015e2cd9d6ff835d4ea545bf9bac4f90cea49): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.2%	3.2%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.3%	-2.3%	1
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	3.6%	4.3%	2
Regressions 😿 (secondary)	8.4%	8.4%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.7%	-3.2%	4
All 😿🎉 (primary)	3.6%	4.3%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

oli-obk · 2022-06-30T08:31:56Z

Yea this seems like the best impl for now to me. We can only make a decision here if we know how the argument will get passed, and that requires asking the backend, which is not information we have or want to have available during mir opts

@bors r+

bors · 2022-06-30T08:31:58Z

📌 Commit 259a7a7 has been approved by oli-obk

bors · 2022-06-30T09:20:56Z

⌛ Testing commit 259a7a7 with merge 7425fb2...

bors · 2022-06-30T12:07:59Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 7425fb2 to master...

rust-timer · 2022-07-01T00:57:21Z

Finished benchmarking commit (7425fb2): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	1.5%	1.5%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	2.2%	2.2%	1
Regressions 😿 (secondary)	2.9%	4.2%	4
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.4%	-2.4%	2
All 😿🎉 (primary)	2.2%	2.2%	1

Cycles

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-3.1%	-3.3%	2
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-3.1%	-3.3%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 22, 2022

rust-highfive assigned davidtwco Jun 22, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2022

davidtwco requested changes Jun 23, 2022

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/block.rs Outdated Show resolved Hide resolved

compiler/rustc_codegen_ssa/src/mir/block.rs Outdated Show resolved Hide resolved

davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2022

davidv1992 added 2 commits June 25, 2022 16:36

Improved naming for copied constant arguments vector.

8f529ab

Added test for const arg lifetimes suggested by erikdesjardins.

259a7a7

davidv1992 force-pushed the add-lifetimes-to-argument-temporaries branch from 6898411 to 259a7a7 Compare June 25, 2022 14:43

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 25, 2022

rust-highfive assigned oli-obk and unassigned davidtwco Jun 28, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 29, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 29, 2022

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 Jun 30, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 30, 2022

bors merged commit 7425fb2 into rust-lang:master Jun 30, 2022

rustbot added this to the 1.64.0 milestone Jun 30, 2022

albertlarsan68 mentioned this pull request Dec 27, 2022

rustc crash #106062

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added llvm lifetime annotations to function call argument temporaries. #98377

Added llvm lifetime annotations to function call argument temporaries. #98377

davidv1992 commented Jun 22, 2022

rust-highfive commented Jun 22, 2022

davidtwco left a comment

erikdesjardins commented Jun 24, 2022 •

edited

Loading

davidv1992 commented Jun 25, 2022

davidv1992 commented Jun 25, 2022

davidtwco commented Jun 28, 2022

oli-obk commented Jun 29, 2022

rust-timer commented Jun 29, 2022

bors commented Jun 29, 2022

oli-obk commented Jun 29, 2022

bors commented Jun 29, 2022

rust-timer commented Jun 29, 2022

rust-timer commented Jun 29, 2022

oli-obk commented Jun 30, 2022

bors commented Jun 30, 2022

bors commented Jun 30, 2022

bors commented Jun 30, 2022

rust-timer commented Jul 1, 2022

Added llvm lifetime annotations to function call argument temporaries. #98377

Added llvm lifetime annotations to function call argument temporaries. #98377

Conversation

davidv1992 commented Jun 22, 2022

rust-highfive commented Jun 22, 2022

davidtwco left a comment

Choose a reason for hiding this comment

erikdesjardins commented Jun 24, 2022 • edited Loading

davidv1992 commented Jun 25, 2022

davidv1992 commented Jun 25, 2022

davidtwco commented Jun 28, 2022

oli-obk commented Jun 29, 2022

rust-timer commented Jun 29, 2022

bors commented Jun 29, 2022

oli-obk commented Jun 29, 2022

bors commented Jun 29, 2022

rust-timer commented Jun 29, 2022

rust-timer commented Jun 29, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

oli-obk commented Jun 30, 2022

bors commented Jun 30, 2022

bors commented Jun 30, 2022

bors commented Jun 30, 2022

rust-timer commented Jul 1, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

erikdesjardins commented Jun 24, 2022 •

edited

Loading