Start using pattern types in libcore#136006
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
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.
ff4d569 to
0409353
Compare
This comment has been minimized.
This comment has been minimized.
0409353 to
4db40b9
Compare
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
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.
|
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.
|
@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.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (72aec09): comparison URL. Overall result: ❌ regressions - 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.4%, secondary 5.8%)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.1%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 480.191s -> 478.416s (-0.37%) |
|
The entire regression is in rustdoc lowering (clean). Unsure why, but it's also very tiny. Likely a more structural issue. Rustdoc clean should not end up seeing any pattern types |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
cc @BoxyUwU |
|
r? compiler |
This comment has been minimized.
This comment has been minimized.
| // While we can't project into `NonNull<_>` in a basic block | ||
| // due to MCP#807, this is debug info where it's fine. | ||
| // While we can't project into a pattern type in a basic block, | ||
| // this is debug info where it's fine. |
There was a problem hiding this comment.
"it's fine" because the pattern type is erased in the debug info?
There was a problem hiding this comment.
No, because debug info has no UB, we can just do whatever works instead of having to worry about soundness
There was a problem hiding this comment.
I think it's less about soundness and more that debug info doesn't need to worry about losing the validity restriction information. We don't want to load an i32 from an i32 is 1.. because that loses the !range metadata, but in debug info whatever.
tests/mir-opt/retag.array_casts.SimplifyCfg-pre-optimizations.after.panic-abort.mir
Outdated
Show resolved
Hide resolved
|
I think this thread from @scottmcm is still open: #136006 (comment) Should we split the compiler and library changes? |
This comment has been minimized.
This comment has been minimized.
I already split everything out that I could. I tried splitting it further but box (and thus NonNull) is just so entangled between the compiler and libcore that it's really hard to do in a way that even some of the compiler changes can be landed and tested |
This comment has been minimized.
This comment has been minimized.
tests/ui/abi/compatibility.rs
Outdated
| test_abi_compatible!(zst_unit, Zst, ()); | ||
| test_abi_compatible!(zst_array, Zst, [u8; 0]); | ||
| test_abi_compatible!(nonzero_int, NonZero<i32>, i32); | ||
| test_abi_compatible!(nonzero_int, pattern_type!(i32 is 1..=0x7FFF_FFFF), i32); |
There was a problem hiding this comment.
Should the pattern types be added as new cases rather than changing the test for NonZero?
There was a problem hiding this comment.
ah, yea that was me being lazy. I changed minicore to support i32 for NonZero.
|
r? wesleywiser |
|
@bors r=wesleywiser |
This comment has been minimized.
This comment has been minimized.
|
@bors r- |
|
@bors r- |
|
Oh huh this PR never got |
There was a problem hiding this comment.
This is looking pretty good 🎉
I'm glad you found a way to get the change down to something pretty small -- when it's more minicore fixes than "real" changes it makes me happier.
EDIT: wow, I missed this had gotten a review. I should go sleep stop looking at code, apparently.
| let dst_ty = dst.layout.ty; | ||
| match (src_ty.kind(), dst_ty.kind()) { | ||
| (&ty::Pat(s, sp), &ty::Pat(d, dp)) | ||
| if let (PatternKind::NotNull, PatternKind::NotNull) = (*sp, *dp) => |
There was a problem hiding this comment.
unsure: does the specific kind of pattern really matter here? Would just
| if let (PatternKind::NotNull, PatternKind::NotNull) = (*sp, *dp) => | |
| if sp == dp => |
be ok?
There was a problem hiding this comment.
Well, if we ever end up with anything but a NotNull, I'm worried, so I like running into the bug! case at the end. Maybe in the future we will support other things, but I'm not sure they should go down the same code path here
| // While we can't project into `NonNull<_>` in a basic block | ||
| // due to MCP#807, this is debug info where it's fine. | ||
| // While we can't project into a pattern type in a basic block, | ||
| // this is debug info where it's fine. |
There was a problem hiding this comment.
I think it's less about soundness and more that debug info doesn't need to worry about losing the validity restriction information. We don't want to load an i32 from an i32 is 1.. because that loses the !range metadata, but in debug info whatever.
|
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. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
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