Start using pattern types in libcore#136006
Open
oli-obk wants to merge 1 commit intorust-lang:mainfrom
Open
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
e3e2cbd to
ff4d569
Compare
compiler-errors
approved these changes
Jan 24, 2025
Contributor
compiler-errors
left a comment
There was a problem hiding this comment.
Well r=me when/if it does get unblocked.
This comment has been minimized.
This comment has been minimized.
scottmcm
reviewed
Jan 25, 2025
ff4d569 to
0409353
Compare
This comment has been minimized.
This comment has been minimized.
0409353 to
4db40b9
Compare
Collaborator
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Member
|
Re rust-analyzer blocker, as it turns out to implement pattern types (to a degree that would unblock this) it comes down to the same architecture changes required as for rust-lang/rust-analyzer#7434 (which I am currently looking into), so that will take a bit. |
This comment has been minimized.
This comment has been minimized.
4db40b9 to
0afd16b
Compare
This comment has been minimized.
This comment has been minimized.
0afd16b to
ed90eb8
Compare
This comment has been minimized.
This comment has been minimized.
ed90eb8 to
9514a86
Compare
This comment has been minimized.
This comment has been minimized.
9514a86 to
9774687
Compare
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
RalfJung
reviewed
Jan 27, 2025
5 tasks
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
@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.
Contributor
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (224f039): 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 2.5%, secondary -2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.5%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 482.691s -> 481.225s (-0.30%) |
Contributor
Author
|
Hell yea @rustbot ready r? compiler |
scottmcm
reviewed
Feb 14, 2026
| // Remember to use `.as_ptr()` instead of `.pointer`, as field projecting to | ||
| // this is banned by <https://github.com/rust-lang/compiler-team/issues/807>. | ||
| pointer: *const T, | ||
| pointer: crate::pattern_type!(*const T is !null), |
Member
There was a problem hiding this comment.
I'm excited to do this, but it feels odd to me to have both cg_ssa changes and the change to the library type in one PR.
Would it maybe be feasible to split this to have zero library changes, just enough new test coverage to justify the compiler updates, then a follow-up no-compiler-review-needed change to the library part?
Member
There was a problem hiding this comment.
hmm, I guess the box elaboration needs to be in the same PR as the library change, doesn't it :/
Contributor
Author
There was a problem hiding this comment.
Yea... I can land some changes ahead of time, but I can only test them with a custom minicore
Contributor
Author
There was a problem hiding this comment.
Gave this another try, I should be able to hit the cg_ssa changes with a test written on rustc today, but I'm missing some necessary component to actually go down this path. I tried https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=72f945c494a52dbb039b2aff32453d41 (and we already have a test that this is a permutation of: https://github.com/rust-lang/rust/blob/main/tests/ui/type/pattern_types/unsize.rs
scottmcm
reviewed
Feb 14, 2026
| _12 = NonNull::<T> { pointer: copy _14 }; | ||
| _15 = copy _14 as (*const T) is !null (Transmute); | ||
| StorageDead(_14); | ||
| _12 = NonNull::<T> { pointer: move _15 }; |
Member
There was a problem hiding this comment.
not this PR's problem: looks like we should either avoid ever using NonNull(blah) in the library because that doesn't fold in GVN or should look at allowing constructors like this to fold with transmutes.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Collaborator
|
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. |
Contributor
Author
|
@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.
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Contributor
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Oh no the changes from #152702 had subtle effects on what LLVM sees when pattern types are used inside NonNull, so now some opts don't work anymore |
Collaborator
|
Finished benchmarking commit (339a92e): 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 0.8%, secondary -0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -1.8%, secondary -2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 479.657s -> 474.169s (-1.14%) |
Contributor
Author
|
Ok that is some nice perf, but is it just costing runtime perf? |
Contributor
Author
basically this PR doesn't change the MIR of pub fn zero(d: &mut [Vec<i32>]) {
let n = d.len();
for i in 0..n {
assert!(d[i].len() == n);
for j in 0..n {
d[i][j] = 0;
}
}
}at all. The MIR of it contains no function calls, it's fully inlined. So the only thing that can cause a regression here is the fact that somewhere we transmute across pattern type and LLVM chokes on that, even tho the pattern type never shows up in MIR. |
Contributor
Author
nope that's not it. I reverted all of it and it didn't affect anything. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
14 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
cc #135996
Replaces the innards of
NonNullwith*const T is !null.This does affect LLVM's optimizations, as now reading the field preserves the metadata that the field is not null, and transmuting to another type (e.g. just a raw pointer), will also preserve that information for optimizations. This can cause LLVM opts to do more work, but it's not guaranteed to produce better machine code.
Once we also remove all uses of rustc_layout_scalar_range_start from rustc itself, we can remove the support for that attribute entirely and handle all such needs via pattern types