Remove redundant information in rustc_abi::Variants#151742
Remove redundant information in rustc_abi::Variants#151742moulins wants to merge 5 commits intorust-lang:mainfrom
rustc_abi::Variants#151742Conversation
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
| #[derive(PartialEq, Eq, Hash, Clone, Debug)] | ||
| #[cfg_attr(feature = "nightly", derive(HashStable_Generic))] | ||
| pub struct VariantLayout<FieldIdx: Idx> { | ||
| pub size: Size, |
There was a problem hiding this comment.
Can't be removed, as it is used by the variant_size_differences lint.
| #[cfg_attr(feature = "nightly", derive(HashStable_Generic))] | ||
| pub struct VariantLayout<FieldIdx: Idx> { | ||
| pub size: Size, | ||
| pub backend_repr: BackendRepr, |
There was a problem hiding this comment.
I don't know enough about codegen to confidently say if it requires accurate reprs for enum variants, so I've left this field for now.
There was a problem hiding this comment.
Enum variants should not need their own BackendRepr, but it is probably a good idea to leave this cleanup for a future PR.
| pub size: Size, | ||
| pub backend_repr: BackendRepr, | ||
| pub field_offsets: IndexVec<FieldIdx, Size>, | ||
| fields_in_memory_order: IndexVec<u32, FieldIdx>, |
There was a problem hiding this comment.
Note: this field could be removed and recomputed from the field offsets in Layout::from_variant; it's unclear whether this is worth it.
| // Remove discriminant values of the other variants from the largest niche. This assumes | ||
| // that the largest niche, when it exists, always corresponds to the enum discriminant. |
There was a problem hiding this comment.
This assumption was already implicitly made by the original code.
| let min = valid_range.start.min(valid_range.end); | ||
| let min = tag.size(cx).truncate(min); | ||
|
|
||
| let max = valid_range.start.max(valid_range.end); | ||
| let max = tag.size(cx).truncate(max); |
There was a problem hiding this comment.
I think this logic is broken for valid ranges wrapping around uN::MAX? In any case, fixing this is out-of-scope of the PR, so I've left it as-is.
|
@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.
Remove redundant information in `rustc_abi::Variants`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (968e542): comparison URL. Overall result: ✅ improvements - 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 1.4%, secondary -2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 470.891s -> 473.88s (0.63%) |
|
cc @workingjubilee IIRC you were involved in the related discussion on discord? |
|
@rustbot reroll |
|
I'm finding another compiler reviewer for this PR. |
|
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. |
|
I don't have access to a Windows machine to debug this properly, so let's try removing the commit that did touch |
|
@bors try jobs=aarch64-msvc-1 |
This comment has been minimized.
This comment has been minimized.
Remove redundant information in `rustc_abi::Variants` try-job: aarch64-msvc-1
|
@bors r+ |
Remove redundant information in `rustc_abi::Variants` Follow-up to rust-lang#151040; partially addresses rust-lang#113988. Replaces the nested `LayoutData` in `Variants::Multiple` by a new, smaller `VariantLayout` struct, and adjust `LayoutData::for_variant`and the layout algorithm in consequence. This PR is best reviewed commit-by-commit.
Remove redundant information in `rustc_abi::Variants` Follow-up to rust-lang#151040; partially addresses rust-lang#113988. Replaces the nested `LayoutData` in `Variants::Multiple` by a new, smaller `VariantLayout` struct, and adjust `LayoutData::for_variant`and the layout algorithm in consequence. This PR is best reviewed commit-by-commit.
Rollup of 9 pull requests Successful merges: - #151742 (Remove redundant information in `rustc_abi::Variants`) - #155856 (std_detect: support detecting more features on aarch64 Windows) - #155861 (Suggest `[const] Trait` bounds in more places) - #155899 (`dlltool`: Set the working directory to workaround `--temp-prefix` bug) - #155916 (Update with new LLVM 22 target for `wasm32-wali-linux-musl` target) - #155935 (remap OUT_DIR paths to fix build script path leakage in crate metadata. ) - #155950 (use the new `//@ needs-asm-mnemonic: ret` more) - #155949 (Update `opt_ast_lowering_delayed_lints` query to allow "stealing" lints, allowing to use `FnOnce` instead of `Fn`) - #155951 (Make `FlatMapInPlaceVec` an unsafe trait.)
This comment has been minimized.
This comment has been minimized.
Remove redundant information in `rustc_abi::Variants` Follow-up to #151040; partially addresses #113988. Replaces the nested `LayoutData` in `Variants::Multiple` by a new, smaller `VariantLayout` struct, and adjust `LayoutData::for_variant`and the layout algorithm in consequence. This PR is best reviewed commit-by-commit.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test for cad8c01 failed: CI. Failed job:
|
|
The rollup failed with the same error, so not spurious |
|
And the try job on the rollup just succeeded, fascinating. I'll leave it to y'all to figure out what to do with this |
|
SIGSEGV from unit tests wat |
|
@bors try jobs=x86_64-gnu-llvm-21-1 |
|
⌛ Trying commit 93db272 with merge 27a14b2… To cancel the try build, run the command Workflow: https://github.com/rust-lang/rust/actions/runs/25113485899 |
Remove redundant information in `rustc_abi::Variants` try-job: x86_64-gnu-llvm-21-1
View all comments
Follow-up to #151040; partially addresses #113988.
Replaces the nested
LayoutDatainVariants::Multipleby a new, smallerVariantLayoutstruct, and adjustLayoutData::for_variantand the layout algorithm in consequence.This PR is best reviewed commit-by-commit.