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
Use the same DISubprogram for each instance of the same inlined function within a caller #114643
Conversation
r? @fee1-dead (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred in compiler/rustc_codegen_gcc cc @antoyo |
This comment has been minimized.
This comment has been minimized.
r? compiler |
f07742a
to
d8a990c
Compare
This comment has been minimized.
This comment has been minimized.
d8a990c
to
a3c9ab2
Compare
This comment has been minimized.
This comment has been minimized.
a3c9ab2
to
f6d3a63
Compare
This comment has been minimized.
This comment has been minimized.
f6d3a63
to
450caba
Compare
@bors try @rust-timer queue |
⌛ Trying commit 450caba2c72fb9909449b7fc2d09f1b47672244c with merge 4f0955dc1384542c3a8e0630c446c01ee2ccbd6c... |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
450caba
to
6c4d4d8
Compare
This comment has been minimized.
This comment has been minimized.
6c4d4d8
to
862a0bc
Compare
@dpaoliello: 🔑 Insufficient privileges: not in try users |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 687bffa with merge bc26305dffb3dc48c9018ff524fe40c907d35b06... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (bc26305dffb3dc48c9018ff524fe40c907d35b06): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking 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 Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeResultsThis 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.
Bootstrap: 632.009s -> 631.987s (-0.00%) |
r? @wesleywiser |
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (154ae32): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis 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.
Bootstrap: 635.929s -> 635.404s (-0.08%) |
Use the same DISubprogram for each instance of the same inlined function within a caller # Issue Details: The call to `panic` within a function like `Option::unwrap` is translated to LLVM as a `tail call` (as it will never return), when multiple calls to the same function like this are inlined LLVM will notice the common `tail call` block (i.e., loading the same panic string + location info and then calling `panic`) and merge them together. When merging these instructions together, LLVM will also attempt to merge the debug locations as well, but this fails (i.e., debug info is dropped) as Rust emits a new `DISubprogram` at each inline site thus LLVM doesn't recognize that these are actually the same function and so thinks that there isn't a common debug location. As an example of this, consider the following program: ```rust #[no_mangle] fn add_numbers(x: &Option<i32>, y: &Option<i32>) -> i32 { let x1 = x.unwrap(); let y1 = y.unwrap(); x1 + y1 } ``` When building for x86_64 Windows using 1.72 it generates (note the lack of `.cv_loc` before the call to `panic`, thus it will be attributed to the same line at the `addq` instruction): ```llvm .cv_loc 0 1 3 0 # src\lib.rs:3:0 addq $40, %rsp retq leaq .Lalloc_f570dea0a53168780ce9a91e67646421(%rip), %rcx leaq .Lalloc_629ace53b7e5b76aaa810d549cc84ea3(%rip), %r8 movl $43, %edx callq _ZN4core9panicking5panic17h12e60b9063f6dee8E int3 ``` # Fix Details: Cache the `DISubprogram` emitted for each inlined function instance within a caller so that this can be reused if that instance is encountered again. Ideally, we would also deduplicate child scopes and variables, however my attempt to do that with rust-lang#114643 resulted in asserts when building for Linux (rust-lang#115156) which would require some deep changes to Rust to fix (rust-lang#115455). Instead, when using an inlined function as a debug scope, we will also create a new child scope such that subsequent child scopes and variables do not collide (from LLVM's perspective). After this change the above assembly now (with <https://reviews.llvm.org/D159226> as well) shows the `panic!` was inlined from `unwrap` in `option.rs` at line 935 into the current function in `lib.rs` at line 0 (line 0 is emitted since it is ambiguous which line to use as there were two inline sites that lead to this same code): ```llvm .cv_loc 0 1 3 0 # src\lib.rs:3:0 addq $40, %rsp retq .cv_inline_site_id 6 within 0 inlined_at 1 0 0 .cv_loc 6 2 935 0 # library\core\src\option.rs:935:0 leaq .Lalloc_5f55955de67e57c79064b537689facea(%rip), %rcx leaq .Lalloc_e741d4de8cb5801e1fd7a6c6795c1559(%rip), %r8 movl $43, %edx callq _ZN4core9panicking5panic17hde1558f32d5b1c04E int3 ```
…wiser Use the same DISubprogram for each instance of the same inlined function within a caller # Issue Details: The call to `panic` within a function like `Option::unwrap` is translated to LLVM as a `tail call` (as it will never return), when multiple calls to the same function like this is inlined LLVM will notice the common `tail call` block (i.e., loading the same panic string + location info and then calling `panic`) and merge them together. When merging these instructions together, LLVM will also attempt to merge the debug locations as well, but this fails (i.e., debug info is dropped) as Rust emits a new `DISubprogram` at each inline site thus LLVM doesn't recognize that these are actually the same function and so thinks that there isn't a common debug location. As an example of this when building for x86_64 Windows (note the lack of `.cv_loc` before the call to `panic`, thus it will be attributed to the same line at the `addq` instruction): ``` .cv_loc 0 1 23 0 # src\lib.rs:23:0 addq $40, %rsp retq leaq .Lalloc_f570dea0a53168780ce9a91e67646421(%rip), %rcx leaq .Lalloc_629ace53b7e5b76aaa810d549cc84ea3(%rip), %r8 movl $43, %edx callq _ZN4core9panicking5panic17h12e60b9063f6dee8E int3 ``` # Fix Details: Cache the `DISubprogram` emitted for each inlined function instance within a caller so that this can be reused if that instance is encountered again, this also requires caching the `DILexicalBlock` and `DIVariable` objects to avoid creating duplicates. After this change the above assembly now looks like: ``` .cv_loc 0 1 23 0 # src\lib.rs:23:0 addq $40, %rsp retq .cv_inline_site_id 5 within 0 inlined_at 1 0 0 .cv_inline_site_id 6 within 5 inlined_at 1 12 0 .cv_loc 6 2 935 0 # library\core\src\option.rs:935:0 leaq .Lalloc_5f55955de67e57c79064b537689facea(%rip), %rcx leaq .Lalloc_e741d4de8cb5801e1fd7a6c6795c1559(%rip), %r8 movl $43, %edx callq _ZN4core9panicking5panic17hde1558f32d5b1c04E int3 ```
Issue Details:
The call to
panic
within a function likeOption::unwrap
is translated to LLVM as atail call
(as it will never return), when multiple calls to the same function like this is inlined LLVM will notice the commontail call
block (i.e., loading the same panic string + location info and then callingpanic
) and merge them together.When merging these instructions together, LLVM will also attempt to merge the debug locations as well, but this fails (i.e., debug info is dropped) as Rust emits a new
DISubprogram
at each inline site thus LLVM doesn't recognize that these are actually the same function and so thinks that there isn't a common debug location.As an example of this when building for x86_64 Windows (note the lack of
.cv_loc
before the call topanic
, thus it will be attributed to the same line at theaddq
instruction):Fix Details:
Cache the
DISubprogram
emitted for each inlined function instance within a caller so that this can be reused if that instance is encountered again, this also requires caching theDILexicalBlock
andDIVariable
objects to avoid creating duplicates.After this change the above assembly now looks like: