Remove redundant information in rustc_abi::Variants#151742
Conversation
|
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. |
|
FYI the ci error was spurious, so feel free to retry when the open comment is resolved |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
For now, this is a 1-to-1 copy of `LayoutData`, but this will change.
It was always set to `Variants::Single`.
Enum variants always have `Arbitrary` layout, so the enum isn't needed.
This field is only used during layout calculations, so re-synthetized `LayoutData`s for enum variants can use a dummy value instead.
Reusing the alignment of the enclosing enum in `LayoutData::for_variant` doesn't appear to cause any issues.
|
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. |
|
@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.
|
@bors try jobs=aarch64-apple |
|
⌛ Trying commit 75015f4 with merge b6014dc… To cancel the try build, run the command Workflow: https://github.com/rust-lang/rust/actions/runs/25990793137 |
Remove redundant information in `rustc_abi::Variants` try-job: aarch64-apple
|
@bors try cancel |
|
Try build cancelled. Cancelled workflows: |
…uwer Rollup of 14 pull requests Successful merges: - #151742 (Remove redundant information in `rustc_abi::Variants`) - #151362 (Add interior-mutability suggestion to `static_mut_refs`) - #156121 (compiler: suggest `.collect()` when `String` is expected and `Iterator` is found) - #156208 (Emit retags in codegen to support BorrowSanitizer (part 1)) - #156596 (Split `LintExpectationId`s) - #156607 (ci: Update FreeBSD version to FreeBSD 14) - #156376 (suggest hex escapes for C-style escapes) - #156577 (Test EII UI tests with prefer-dynamic) - #156585 (explicit tail calls: ignore some tests on unsupported LLVM targets) - #156598 (Avoid rustfix suggestions for macro-expanded unused imports) - #156616 (rustdoc: add test case for `-Drustdoc::` and `--cap-lints`) - #156633 (Add regression test for issue #41261) - #156635 (rename unexpected_try_recover function) - #156636 (minor `rustc_mir_transform` cleanup)
…uwer Rollup of 14 pull requests Successful merges: - #151742 (Remove redundant information in `rustc_abi::Variants`) - #151362 (Add interior-mutability suggestion to `static_mut_refs`) - #156121 (compiler: suggest `.collect()` when `String` is expected and `Iterator` is found) - #156208 (Emit retags in codegen to support BorrowSanitizer (part 1)) - #156596 (Split `LintExpectationId`s) - #156607 (ci: Update FreeBSD version to FreeBSD 14) - #156376 (suggest hex escapes for C-style escapes) - #156577 (Test EII UI tests with prefer-dynamic) - #156585 (explicit tail calls: ignore some tests on unsupported LLVM targets) - #156598 (Avoid rustfix suggestions for macro-expanded unused imports) - #156616 (rustdoc: add test case for `-Drustdoc::` and `--cap-lints`) - #156633 (Add regression test for issue #41261) - #156635 (rename unexpected_try_recover function) - #156636 (minor `rustc_mir_transform` cleanup)
Rollup merge of #151742 - moulins:variant-layout, r=saethlin 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.
…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#151742 (Remove redundant information in `rustc_abi::Variants`) - rust-lang/rust#151362 (Add interior-mutability suggestion to `static_mut_refs`) - rust-lang/rust#156121 (compiler: suggest `.collect()` when `String` is expected and `Iterator` is found) - rust-lang/rust#156208 (Emit retags in codegen to support BorrowSanitizer (part 1)) - rust-lang/rust#156596 (Split `LintExpectationId`s) - rust-lang/rust#156607 (ci: Update FreeBSD version to FreeBSD 14) - rust-lang/rust#156376 (suggest hex escapes for C-style escapes) - rust-lang/rust#156577 (Test EII UI tests with prefer-dynamic) - rust-lang/rust#156585 (explicit tail calls: ignore some tests on unsupported LLVM targets) - rust-lang/rust#156598 (Avoid rustfix suggestions for macro-expanded unused imports) - rust-lang/rust#156616 (rustdoc: add test case for `-Drustdoc::` and `--cap-lints`) - rust-lang/rust#156633 (Add regression test for issue rust-lang/rust#41261) - rust-lang/rust#156635 (rename unexpected_try_recover function) - rust-lang/rust#156636 (minor `rustc_mir_transform` cleanup)
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.