-
Notifications
You must be signed in to change notification settings - Fork 13.8k
GVN: Use a VnIndex in Address projection. #144477
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
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (413faf3): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never 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 0.1%, secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 468.925s -> 469.927s (0.21%) |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
GVN: Use a VnIndex in Address projection.
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (62808a4): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never 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 1.2%, secondary 2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary -0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 465.394s -> 465.423s (0.01%) |
☔ The latest upstream changes (presumably #145773) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #146289) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors try @rust-timer queue |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
☔ The latest upstream changes (presumably #145737) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
r? @dianqk Since they should be more familiar with gvn :) |
Requested reviewer is already assigned to this pull request. Please choose another assignee. |
This PR was rebased onto a different master 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. |
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.
LGTM.
r=me
// CHECK-LABEL: fn repeat_local( | ||
// CHECK: _0 = copy _3 | ||
// CHECK: let mut [[ELEM:.*]]: &i32; | ||
// CHECK: _0 = copy (*[[ELEM]]); |
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.
Should we leave a FIXME here?
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.
The new result is more correct, so I'd rather not. The current annotation was more of a bug: idx1
is not SSA, so we should never have been able to refer to &array[idx1]
.
@bors r=dianqk |
☀️ Test successful - checks-actions |
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 8111a2d (parent) -> 3d8c1c1 (this PR) Test differencesShow 2 test diffs2 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 3d8c1c1fc077d04658de63261d8ce2903546db13 --output-dir test-dashboard And 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 (3d8c1c1): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@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 5.1%, secondary 0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 471.909s -> 472.01s (0.02%) |
The current implementation of address projections is inconsistent. Indexing semantically relies on the index' value, but the implementation uses the index' place. This PR fixes that by using
ProjectionElem<VnIndex, Ty<'tcx>>
instead of the rawPlaceElem<'tcx>
.This is a more principled fix than the workaround in #145030.