-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
avoid phi node for pointers flowing into Vec appends #130998
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
Conversation
|
On its own I don't expect this to do much, we also need llvm/llvm-project#110280 to get memcpy propagation. But lets see what the perf impact is without the LLVM changes. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
avoid phi node for pointers flowing into Vec appends related discussion: https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/nocapture.20and.20allocation.20elimination r? ghost
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (fa341e6): comparison URL. Overall result: ❌✅ regressions and improvements - 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @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)Results (primary -0.1%, secondary 0.5%)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.
CyclesResults (secondary -0.3%)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.
Binary sizeResults (primary 0.0%, 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.
Bootstrap: 769.092s -> 770.401s (0.17%) |
|
As expected it doesn't do much on its own, let's wait for the LLVM change. |
0eec4ba to
5bba169
Compare
This comment has been minimized.
This comment has been minimized.
5bba169 to
c44e0f4
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
avoid phi node for pointers flowing into Vec appends related discussion: https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/nocapture.20and.20allocation.20elimination r? ghost
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (fd59d69): comparison URL. Overall result: ❌✅ regressions and improvements - 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @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)Results (primary 2.5%)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.
CyclesResults (secondary -2.8%)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.
Binary sizeResults (primary 0.0%, secondary 0.1%)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.
Bootstrap: 770.125s -> 769.973s (-0.02%) |
|
waiting for #135763 |
|
#135763 is now merged so unblocking |
|
r? compiler |
| // applies to argument place instead of function place | ||
| let allocated_pointer = AttributeKind::AllocatedPointer.create_attr(cx.llcx); | ||
| attributes::apply_to_llfn(llfn, AttributePlace::Argument(0), &[allocated_pointer]); | ||
| let attrs: &[_] = if llvm_util::get_version() >= (21, 0, 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have some sort of justification for why this is correct. This operation destroys the provenance given to it, why is it okay to consider that non-capturing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding from the zulip conversation is that since the allocation ends there's nothing further that the callee could do that would be relevant to aliasing rules of this allocation.
Whether or not the allocator touches the memory afterwards (e.g. for zeroing), isn't relevant to this side of the allocator boundary.
I assume we need to tell LLVMs both things separately because they just happen to be tracked separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic just to check the obvious, adding this attribute wouldn't allow
let p = alloc();
foo(p);
dealloc(p);
to be reordered to
let p = alloc();
dealloc(p);
foo(p);
Being an allocator method already inhibits that reordering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Does not capture provenance" means "if the function call stashes the pointer somewhere, accessing that pointer after the function returns is UB". It does not limit what can be done with the pointer within the function itself.
FWIW, the C free function is marked captures(none).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we shouldn't match what C is doing then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that if GlobalAlloc is used, it may inspect the address of the pointer, so using captures(none) may not be correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let attrs: &[_] = if llvm_util::get_version() >= (21, 0, 0) { | |
| let attrs: &[_] = if llvm_util::get_version() >= (21, 0, 0) { | |
| // "Does not capture provenance" means "if the function call stashes the pointer somewhere, | |
| // accessing that pointer after the function returns is UB". That is definitely the case here since | |
| // freeing will destroy the provenance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that if GlobalAlloc is used, it may inspect the address of the pointer, so using captures(none) may not be correct.
But C allocators would also have to look at the address (e.g. comparing it to memory pools). Is the difference that those are assumed to be compiled separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we care about here are effects that are observable outside the allocator. Specifically, what I had in mind is GlobalAlloc::dealloc() doing something like print "I'm now freeing pointer 0xdeadbeef".
Co-authored-by: Ralf Jung <post@ralfj.de>
f4a2ee3 to
468eb45
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
This is way outside my wheelhouse, but the interactions with Ralf and Nikita make it seem like it's ok... @bors r+ |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 8c52f73 (parent) -> 86a49fd (this PR) Test differencesShow 199 test diffsStage 1
Stage 2
Additionally, 194 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 86a49fd71fecd25b0fd20247db0ba95eeceaba28 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (86a49fd): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 7.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.6%, secondary -3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 472.286s -> 475.838s (0.75%) |
|
The new codegen test appears to have been causing flaky CI failures: |
…g#130998" This reverts PR <rust-lang#130998> because the added test seems to be flaky / non-deterministic, and has been failing in unrelated PRs during merge CI.
Revert "avoid phi node for pointers flowing into Vec appends #130998" This reverts PR #130998 because the added test seems to be flaky / non-deterministic, and has been failing in unrelated PRs during merge CI: - #151129 (comment) - #150772 (comment) - #150925 (comment) See also [#t-infra > Tree ops](https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Tree.20ops/with/568111767). > [!NOTE] > > This is a "fallback" PR in case the FileCheck failure isn't obvious (i.e. fix-forward). This PR reverts #130998 wholesale in case the failure is genuine and indicative of a bug in the actual implementation change.
Revert "avoid phi node for pointers flowing into Vec appends rust-lang#130998" This reverts PR rust-lang#130998 because the added test seems to be flaky / non-deterministic, and has been failing in unrelated PRs during merge CI: - rust-lang#151129 (comment) - rust-lang#150772 (comment) - rust-lang#150925 (comment) - rust-lang#151145 (comment) See also [#t-infra > Tree ops](https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Tree.20ops/with/568111767). > [!NOTE] > > This is a "fallback" PR in case the FileCheck failure isn't obvious (i.e. fix-forward). This PR reverts rust-lang#130998 wholesale in case the failure is genuine and indicative of a bug in the actual implementation change.
Rollup merge of #151150 - revert-vec-append, r=Zalathar Revert "avoid phi node for pointers flowing into Vec appends #130998" This reverts PR #130998 because the added test seems to be flaky / non-deterministic, and has been failing in unrelated PRs during merge CI: - #151129 (comment) - #150772 (comment) - #150925 (comment) - #151145 (comment) See also [#t-infra > Tree ops](https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/Tree.20ops/with/568111767). > [!NOTE] > > This is a "fallback" PR in case the FileCheck failure isn't obvious (i.e. fix-forward). This PR reverts #130998 wholesale in case the failure is genuine and indicative of a bug in the actual implementation change.
…ng#130998" This reverts commit cd79ff2.
Elide temporary allocations in patterns like
vec.append(slice.to_vec())related discussion: https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/nocapture.20and.20allocation.20elimination