Pick the largest niche even if the largest niche is wrapped around #144577

oli-obk · 2025-07-28T10:30:19Z

oli-obk · 2025-07-28T10:31:00Z

tests/ui/transmutability/enums/niche_optimization.rs

    assert::is_transmutable::<OptionLike, u8>();
    assert::is_transmutable::<V0, OptionLike>();
+    assert::is_transmutable::<V1, OptionLike>();
    assert::is_transmutable::<V254, OptionLike>();
-    assert::is_transmutable::<V255, OptionLike>();


@jswrenn I'm not sure how to properly fix this test to check what it was checking before. I guess I could create enums with all but a few variants?

So long as you're confident that the revised test will fail if the niche selection process is changed, I'm not too opinionated about the exact approach.

Ah oops. Should have done this.

Will create a PR, but it may take 4 weeks as I'm going on vacation

No problem; happy to review it when you do so!

oli-obk · 2025-07-28T10:32:01Z

compiler/rustc_middle/src/ty/layout.rs

-        // If there are no negative values, we can use the unsigned fit.
-        if min >= 0 {
+        // Pick the smallest fit.
+        if unsigned_fit <= signed_fit {


This affected no tests on its own (but is needed for this PR, as previously it was assumed that min < max), but I think it is strictly better, I'm not expecting signed ops to be slower or anything

(Signed ops can actually optimize better, even, since sub nuw %x, 1 turns into add %x, -1 whereas sub nsw %x, 1 keeps its nowrap flag becoming add nsw %x, -1.)

But yeah, if it's the same integer size either way it probably doesn't matter which is chosen.

scottmcm · 2025-07-29T05:36:51Z

compiler/rustc_abi/src/layout.rs

+            .map(|(_, val)| {
+                if discr_type.is_signed() {
+                    // sign extend the raw representation to be an i128
+                    (val << (128 - bits)) >> (128 - bits)


I think you have the discr_int.size(), so you can do https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.Size.html#method.sign_extend instead of writing this out.

good point. it was preexisting, so I'll pack it into a separate commit.

compiler/rustc_abi/src/layout.rs

scottmcm · 2025-07-29T05:42:03Z

compiler/rustc_abi/src/layout.rs

+        // Since they were sorted, we can now compute the niche sizes and pick the largest.
+        let discriminants = discriminants.zip(next_discriminants);
+        let largest_niche = discriminants.max_by_key(|&(start, end)| {
+            if start <= end { end - start } else { discr_int.signed_max() - (start - end) }


Hmm, I'm kinda surprised to see the start <= end check here. Since we're calculating a distance in a wrapping range, could it be end.wrapping_sub(start)? Or I guess if we need to trim that to the type in question, size.truncate(end.wrapping_sub(start))?

(There's also something going weird here in general because the CI failure is here.)

CI failure is only for 128 bit discriminants with niches larger than i128::MAX because those turn negative

Got a fix and comments locally, just forgot to push

scottmcm · 2025-07-29T19:16:15Z

Thanks for fixing! Looks good.
@bors r+

bors · 2025-07-29T19:16:18Z

📌 Commit 219bad4 has been approved by scottmcm

It is now in the queue for this repository.

bors · 2025-07-30T02:57:08Z

⌛ Testing commit 219bad4 with merge 919c409...

bors · 2025-07-30T05:58:59Z

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 919c409 to master...

github-actions · 2025-07-30T06:02:15Z

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 c8bb4e8 (parent) -> 919c409 (this PR)

Test differences

Show 6 test diffs

Stage 1

[ui] tests/ui/enum-discriminant/wrapping_niche.rs: [missing] -> pass (J1)

Stage 2

[ui] tests/ui/enum-discriminant/wrapping_niche.rs: [missing] -> pass (J0)

Additionally, 4 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 919c40924373c248315a76618d8c52d39aed5e6e --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

dist-aarch64-msvc: 8158.0s -> 4916.4s (-39.7%)
aarch64-apple: 6746.8s -> 5145.3s (-23.7%)
x86_64-apple-1: 7963.5s -> 6973.0s (-12.4%)
aarch64-gnu-debug: 3968.9s -> 4397.6s (10.8%)
x86_64-apple-2: 5552.5s -> 6111.8s (10.1%)
x86_64-gnu-nopt: 8220.9s -> 7455.1s (-9.3%)
aarch64-gnu: 6634.7s -> 7158.8s (7.9%)
dist-ohos-aarch64: 4043.7s -> 4342.6s (7.4%)
x86_64-rust-for-linux: 3187.5s -> 2953.4s (-7.3%)
dist-apple-various: 4493.2s -> 4817.7s (7.2%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-07-30T07:09:56Z

Finished benchmarking commit (919c409): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -1.6%, secondary 0.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.8%	[0.8%, 0.8%]	1
Improvements ✅ (primary)	-1.6%	[-1.8%, -1.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.6%	[-1.8%, -1.5%]	2

Cycles

Results (secondary 1.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.3%	[1.3%, 1.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 470.143s -> 468.849s (-0.28%)
Artifact size: 376.75 MiB -> 376.81 MiB (0.02%)

Add regression test

45b0c8d

rustbot assigned scottmcm Jul 28, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 28, 2025

oli-obk commented Jul 28, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

scottmcm reviewed Jul 29, 2025

View reviewed changes

compiler/rustc_abi/src/layout.rs Show resolved Hide resolved

scottmcm reviewed Jul 29, 2025

View reviewed changes

oli-obk added 2 commits July 29, 2025 14:08

Pick the largest niche even if the largest niche is wrapped around

75bdbf2

Reuse sign_extend helper

219bad4

oli-obk force-pushed the wrapping-niche branch from e400158 to 219bad4 Compare July 29, 2025 14:18

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 30, 2025

bors merged commit 919c409 into rust-lang:master Jul 30, 2025
11 checks passed

rustbot added this to the 1.90.0 milestone Jul 30, 2025

Pick the largest niche even if the largest niche is wrapped around #144577

Pick the largest niche even if the largest niche is wrapped around #144577

Uh oh!

Conversation

oli-obk commented Jul 28, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottmcm commented Jul 29, 2025

Uh oh!

bors commented Jul 29, 2025

Uh oh!

bors commented Jul 30, 2025

Uh oh!

bors commented Jul 30, 2025

Uh oh!

Uh oh!

github-actions bot commented Jul 30, 2025

Test differences

Stage 1

Stage 2

Job duration changes

Uh oh!

rust-timer commented Jul 30, 2025

Overall result: no relevant changes - no action needed

Uh oh!

Uh oh!

oli-obk commented Jul 28, 2025 •

edited by rustbot

Loading

oli-obk Jul 28, 2025 •

edited

Loading