Do not attempt to commute comparison and cast to codegen discriminants

[cjgillot]

The general algorithm to compute a discriminant is:

relative_tag = tag - niche_start
is_niche = relative_tag <= (ule) relative_max
discr = if is_niche {
    cast(relative_tag) + niche_variants.start()
} else {
    untagged_variant
}

We have an optimization branch which attempts to merge the addition and the subtraction by commuting them with the cast. We currently get this optimization wrong.

This PR takes the easiest and safest way: remove the optimization, and let LLVM handle it. (Perf may not agree with that course of action 😅)

There may be a less invasive solution, but I don't have the necessary knowledge of LLVM semantics to find it. Cranelift has the same optimization, which should be handled similarly.
cc @nikic and @bjorn3 if you have a better solution.

Fixes #110128

[rustbot]

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[b-naber]

r? @nikic

[cjgillot]

@bors try @rust-timer queue

[bors]

⌛ Trying commit c92f9c97a31ea8aca28348ad4c1143e30e50a2d0 with merge a6684b582eded5f15890c6cf71312ac8a0fe555f...

[bors]

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

[nikic]

cc @mikebenfield who implemented this code in #102872 and #104535.

[rust-timer]

Finished benchmarking commit (a6684b582eded5f15890c6cf71312ac8a0fe555f): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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 may lead 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-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.5%, 0.7%]	2
Regressions ❌ (secondary)	1.5%	[0.3%, 2.5%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.5%, 0.7%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.8%, -0.4%]	3
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-0.3%	[-0.8%, 0.6%]	4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.6%, 2.1%]	2
Improvements ✅ (primary)	-0.6%	[-1.1%, -0.4%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-1.1%, -0.4%]	7

[pnkfelix]

@bors r+

It is clearly safe (if less efficient) to remove the special case branches as is done here, and the hit to compiler performance is acceptable given that it is fixing a soundness bug.

Let think about getting this into the beta if we don't land it before the beta-branch of nightly (which will be happening either today or tomorrow is I understand correctly). So I'll beta-nom this ahead of time so that it doesn't get forgotten.

@rustbot label: beta-nominated

(And of course, anyone should feel free to try to readd a corrected version of this code, but definitely double check the cases described by #110128)

[bors]

📌 Commit 7c43b7011725d2898f15de6d7c70bdb414162234 has been approved by pnkfelix

It is now in the queue for this repository.

[bors]

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

1. git checkout codegen-discr (switch to your branch)
2. git fetch upstream master (retrieve the latest master)
3. git rebase upstream/master -p (rebase on top of it)
4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
5. git push self codegen-discr --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging src/tools/tidy/src/ui_tests.rs
CONFLICT (content): Merge conflict in src/tools/tidy/src/ui_tests.rs
Automatic merge failed; fix conflicts and then commit the result.

[bors]

☔ The latest upstream changes (presumably #109875) made this pull request unmergeable. Please resolve the merge conflicts.

[cjgillot]

@bors r=pnkfelix

[bors]

📌 Commit a8857d2 has been approved by pnkfelix

It is now in the queue for this repository.

[bors]

⌛ Testing commit a8857d2 with merge 158c309...

[bors]

☀️ Test successful - checks-actions
Approved by: pnkfelix
Pushing 158c309 to master...

[rust-timer]

Finished benchmarking commit (158c309): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.4%	[0.3%, 2.5%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-3.4%, -2.2%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

[pietroalbini]

Removed the beta nomination as it landed in 1.70 already.