test - simulate default discriminant start at 1

[bonega]

WIP/TEST
Trying to gauge the general performance impact of enums not starting with zero.
This is only for repr(rust)

Please sate my curiosity with a perf-run

Edit: sorry - didn't mean to set of all the sirens...

[rust-highfive]

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993).

cc @eddyb

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

[rust-highfive]

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

[rust-highfive]

⚠️ Warning ⚠️

• These commits modify compiler targets. (See the Target Tier Policy.)

[the8472]

Wouldn't it be simpler to adjust the compiler logic that determines the implicit discriminants when no explicit ones are given? That'll also catch those in external crates.

[bonega]

There are a couple of enums in the compiler that expects to have discriminant zero, without it being explicit.
So had to manually go sort them out, that was fun.

My first try to change the logic for default discriminant didn't work either. Can't remember if it was because of before mentioned reason or not

Should have another go at it now.

[eddyb]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit d1114f32699b93b71f26be21450fe73a1e5950bc with merge b5182a3a0771fd7758d02a252cd70807f9e268d3...

[bors]

☀️ Try build successful - checks-actions
Build commit: b5182a3a0771fd7758d02a252cd70807f9e268d3 (b5182a3a0771fd7758d02a252cd70807f9e268d3)

[rust-timer]

Queued b5182a3a0771fd7758d02a252cd70807f9e268d3 with parent 2c7bc5e, future comparison URL.

[rust-timer]

Finished benchmarking commit (b5182a3a0771fd7758d02a252cd70807f9e268d3): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

• Moderate improvement in instruction counts (up to -0.7% on incr-unchanged builds of webrender-wrench)
• Moderate regression in instruction counts (up to 1.6% on incr-patched: println builds of html5ever)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[bonega]

Thanks for the perf-run.

I tried to change initial_discriminant function to val: 1 and disable overflow check in wrap_incr.
I hoped that would mean that only overflow-tests would break.

Get a lot of coredumps instead.
Any tips for this?

[oli-obk]

disable overflow check in wrap_incr.

What does this mean?

I would have assumed we'd need to remove the -1 in

rust/compiler/rustc_middle/src/ty/util.rs

Line 71 in cdeba02

let val = if oflo { min + (n - (max - val) - 1) } else { val + n };

and

rust/compiler/rustc_middle/src/ty/util.rs

Line 80 in cdeba02

let val = if oflo { n - (max - val) - 1 } else { val + n };

so that we essentially do +1 on overflow. There may be more sites to touch, but I would hope this would be the only place to touch

[bonega]

Thanks!

But I don't want to forbid zero as discriminant.
Sorry for the confusing title, my bad - Changing it.
I want to start non explicit discriminant selection at 1.
If it wraps around to 0 then that is fine.

I think your change would make it so that we can only fit 255 variants in an repr(u8)?

Anyhow the change still seem to result in same problem: Lot of coredumps

[oli-obk]

right, I misunderstood the intention. Just wrapping around normally is good then. Can you push your changes somewhere so we can be sure to be talking about the same things?

[bonega]

Pushed my best guess.

Intention: For repr(rust) enums with no explicit discriminants, use 1 instead of 0 for initial discriminant.

I get a lot of coredumps for this.
Unsure how to best debug/structure it.
Thankful for any tips

[oli-obk]

my recommendation for debugging it is putting it behind a feature gate and then creating a very simple test.

compile the test once with the feature and once without, but with explicit discriminants that should be equal and compare various tracing dumps between the two runs to see how things differ.

[tmiasko]

What is motivation behind an additional optimize argument to AdtDef::discriminants? In commit above, the MIR building uses false, while uninhabited enum branches through discriminant_for_variant will potentially use true, so at least one arm of every match on an enum using the optimization will be completely removed out at MIR level :).

I would also look at codegen / interpreter of niche encoding, if I recall correctly it assumes that discriminants start at 0.

[bonega]

Thanks to both of you.

I have pushed a new version that only has one test failing for /x.py test ui.
rust/src/test/ui/mir/issue-76803-branches-not-same.rs.
Will think about it some more

[bonega]

Good catch.
I was thinking it was my code...

[bonega]

x.py test --stage1 and x.py build --stage1 seems to work.
Stage2 blows up though.

@tmiasko I looked at codegen / interpreter of niche encoding, but didn't find anything apparent about discriminant zero.
Can you remember anything more about this?

[tmiasko]

The most recent implementation excludes enums with variants containing fields from optimization, so this should eliminate the niche encoding from consideration, shouldn't it? You could additionally verify that with an assertion that discriminant_for_variant(..., Variant::new(0)) is always zero when using niche encoding in codegen / interpreter.

Regarding the errors from the CI, the DepKind enum from define_dep_nodes macro would be my guess. It is used as an index. Although, locally I am getting a different kind of issue altogether - undefined references during linking, looks like a mismatch in symbol hashes, hmm.

[bonega]

You are right about niche encoding.
At the moment I am still stuck with undefined references.

[JohnCSimon]

triage:
@bonega Can you please address the build failures and adjust the label with @rustbot ready when you're ready for review.

[bonega]

Been stuck on this for a while, will try and look at it again

[JohnCSimon]

Ping from triage:
@bonega
I'm closing this due to inactivity, Please reopen when you are ready to continue with this. Thank you.

@rustbot label: +S-inactive