Fix insertion of statements to be executed along return edge in inlining #117783
Conversation
|
r? @davidtwco (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
Fix insertion of statements to be executed along return edge in inlining Inlining creates additional statements to be executed along the return edge: an assignment to the destination, storage end for temporaries. Previously those statements where inserted directly into a call target, but this is incorrect when the target has other predecessors. Avoid the issue by creating a new dedicated block for those statements. When the block happens to be redundant it will be removed by CFG simplification that follows inlining. Fixes rust-lang#117355
| }; | ||
|
|
||
| let return_block = if let Some(block) = target { | ||
| // Prepare a new block for code that should execute when call returns. We don't use |
There was a problem hiding this comment.
Should we attempt to avoid creating this new block if target has only a single predecessor? I started perf to verify whether there is anything to gain.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (af26da3): comparison URL. Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric. 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 sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 674.708s -> 673.584s (-0.17%) |
rustbot
removed
the
S-waiting-on-perf
|
Apparently no perf to gain. |
bors
added
S-waiting-on-bors
Fix insertion of statements to be executed along return edge in inlining Inlining creates additional statements to be executed along the return edge: an assignment to the destination, storage end for temporaries. Previously those statements where inserted directly into a call target, but this is incorrect when the target has other predecessors. Avoid the issue by creating a new dedicated block for those statements. When the block happens to be redundant it will be removed by CFG simplification that follows inlining. Fixes rust-lang#117355
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
cjgillot
added
S-waiting-on-author
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
@rustbot ready |
rustbot
added
S-waiting-on-review
|
@bors r+ |
bors
added
S-waiting-on-bors
Fix insertion of statements to be executed along return edge in inlining Inlining creates additional statements to be executed along the return edge: an assignment to the destination, storage end for temporaries. Previously those statements where inserted directly into a call target, but this is incorrect when the target has other predecessors. Avoid the issue by creating a new dedicated block for those statements. When the block happens to be redundant it will be removed by CFG simplification that follows inlining. Fixes rust-lang#117355
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
Inlining creates additional statements to be executed along the return edge: an assignment to the destination, storage end for temporaries. Previously those statements where inserted directly into a call target, but this is incorrect when the target has other predecessors. Avoid the issue by creating a new dedicated block for those statements. When the block happens to be redundant it will be removed by CFG simplification that follows inlining. Fixes rust-lang#117355
@bors r=cjgillot |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (79e961f): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. 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: 677.224s -> 674.774s (-0.36%) |
Inlining creates additional statements to be executed along the return
edge: an assignment to the destination, storage end for temporaries.
Previously those statements where inserted directly into a call target,
but this is incorrect when the target has other predecessors.
Avoid the issue by creating a new dedicated block for those statements.
When the block happens to be redundant it will be removed by CFG
simplification that follows inlining.
Fixes #117355