[codegen] assume the tag, not the relative discriminant

[scottmcm]

Address the issue mentioned in llvm/llvm-project#134024 (comment) by changing discriminant calculation to assume on the originally-loaded tag, rather than on cast(tag)-OFFSET.

The previous way does make the purpose of the assume clearer, IMHO, since you see assume(x != 4); if p { x } else { 4 }, but doing it this way instead means that the adds optimize away in LLVM21, which is more important. And this new way is still easily thought of as being like metadata on the load saying specifically which value is impossible.

Demo of the LLVM20 vs LLVM21 difference: https://llvm.godbolt.org/z/n54x5Mq1T

r? @nikic

[rustbot]

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

[nikic]

Wouldn't this select be at the bottom for LLVM 21?

[scottmcm]

Oops, I completely missed that! Thanks.

[nikic]

This looks like a reasonable thing to do to me, but I'm probably the wrong person to check whether the logic is correct. The surrounding niche/discriminant logic looks kinda tricky.

[scottmcm]

r? codegen

[scottmcm]

In case this helps review, here's how you can see this just from the diff, rather than from https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.TagEncoding.html#variant.Niche:

Previously we were doing tagged_discr != untagged_variant here.

But

relative_discr = tag - niche_start
delta = niche_variants.start()
tagged_discr  = relative_discr + delta

so

   tagged_discr != untagged_variant
=> relative_discr + delta != untagged_variant
=> (tag - niche_start) + niche_variants.start() != untagged_variant
=> tag != niche_start  + untagged_variant - niche_variants.start() 

which is the calculation on line 522.

[WaffleLapkin]

@bors r+

[bors]

📌 Commit b90e0fb has been approved by WaffleLapkin

It is now in the queue for this repository.

[scottmcm]

Rebased now that LLVM21 landed, and found locally that mid_bool_eq_discr and mid_ord_eq_discr needed some test updates too. Will r= after CI passes.

[scottmcm]

@bors r=WaffleLapkin rollup=iffy (LLVM21 isn't checked in the PR build, so this might have codegen test failures)

[bors]

📌 Commit c396521 has been approved by WaffleLapkin

It is now in the queue for this repository.

[bors]

⌛ Testing commit c396521 with merge 2886b36...

[scottmcm]

Out of curiosity, from #145077:
@rust-timer build e1b67b6

[rust-timer]

Finished benchmarking commit (e1b67b6): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking 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
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.4%	[0.4%, 0.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.3%]	10
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.5%, secondary 2.4%)

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)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Cycles

Results (secondary 2.4%)

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)	2.4%	[2.4%, 2.4%]	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: 464.119s -> 464.902s (0.17%)
Artifact size: 377.50 MiB -> 377.45 MiB (-0.01%)