[Perf experiment] Make checked unsigned add not use intrinsics in debug mode

[FractalFir]

NOT MEANT FOR MERGING YET

This PR replaces unsigned checked addition LLVM intrinsic with simpler, inline implementations in debug mode.

Motivation

According to the LLVM performance tips, arithmetics intrinsic (including overflow checks) should be avoided if possible, since the optimizer is not as good at reasoning about them:

Avoid using arithmetic intrinsics unless you are required by your source language specification to emit a particular code sequence. The optimizer is quite good at reasoning about general control flow and arithmetic, it is not anywhere near as strong at reasoning about the various intrinsics. If profitable for code generation purposes, the optimizer will likely form the intrinsics itself late in the optimization pipeline. It is very rarely profitable to emit these directly in the language frontend. This item explicitly includes the use of the overflow intrinsics.

The goal of this PR is to check if using a simple, inline implementation of overflow checks is beneficial for compile times, compared to just using the intrinsic.

From my research, in debug mode, the runtime performance of manual overflow checks is comparable to the performance of intrinsics.

I will perform a perf run to see if:

1. This PR improves compiletimes.
2. This PR does not degrade debug runtime performance in a noticeable way.

Implementation

The inline overflow checks are performed by comparing the sum of two numbers to the result of or-ing them together.
The sum a + b is smaller than a | b if and only if an overflow occurs. This is used to detect overflows.

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[FractalFir]

@bors try @rust-timer queue

[bors]

⌛ Trying commit ffc4850 with merge b96f962...

[bors]

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

[hanna-kruppe]

The inline overflow checks are performed by comparing the sum of two numbers to the result of or-ing them together. The sum a + b is smaller than a | b if and only if an overflow occurs. This is used to detect overflows.

Isn't it enough to compare the sum to either of the operands? That is, a < a + b or b < a + b.

[FractalFir]

The inline overflow checks are performed by comparing the sum of two numbers to the result of or-ing them together. The sum a + b is smaller than a | b if and only if an overflow occurs. This is used to detect overflows.

Isn't it enough to compare the sum to either of the operands? That is, a < a + b or b < a + b.

Seem so... it definetly holds for u8, and u16, so I assume it should work for all unsigned intiegers too. I'll change that, and then do a perf run.

[FractalFir]

a + b < a also seems to be the canonical form of the intrinsics, and causes the LLVM backend to reform it when profitable.

https://godbolt.org/z/PY53ax5PY

https://github.com/llvm/llvm-project/blob/f3390fca034d2762f87ac8057078d6e9661305ce/llvm/lib/CodeGen/CodeGenPrepare.cpp#L1669

So, this should also work fine in release.

[FractalFir]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 721c592 with merge 6021669...

[lqd]

@rust-timer build b96f962

[rust-timer]

Finished benchmarking commit (b96f962): comparison URL.

Overall result: ❌ regressions - no 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.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

Results (primary -2.2%, secondary -2.6%)

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)	-2.2%	[-2.9%, -0.9%]	5
Improvements ✅ (secondary)	-2.6%	[-4.0%, -2.0%]	34
All ❌✅ (primary)	-2.2%	[-2.9%, -0.9%]	5

Cycles

Results (secondary -3.0%)

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)	-3.0%	[-3.8%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.0%)

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.0%	[0.0%, 0.0%]	10
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	3
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	8
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.0%, 0.0%]	18

Bootstrap: 765.734s -> 766.119s (0.05%)
Artifact size: 362.10 MiB -> 362.06 MiB (-0.01%)

[bors]

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

[FractalFir]

Maybe the cannonical version will do better?

@rust-timer build 6021669

[rust-timer]

Finished benchmarking commit (6021669): comparison URL.

Overall result: ❌ regressions - please read the text below

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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.4%, 0.5%]	2
Regressions ❌ (secondary)	0.5%	[0.2%, 1.0%]	18
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.8%]	1
All ❌✅ (primary)	0.5%	[0.4%, 0.5%]	2

Max RSS (memory usage)

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

Cycles

Results (primary -1.6%, secondary 0.4%)

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.1%	[2.1%, 2.1%]	2
Improvements ✅ (primary)	-1.6%	[-1.8%, -1.3%]	2
Improvements ✅ (secondary)	-1.4%	[-1.6%, -1.1%]	2
All ❌✅ (primary)	-1.6%	[-1.8%, -1.3%]	2

Binary size

Results (primary 0.0%, secondary 0.0%)

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.0%	[0.0%, 0.1%]	21
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	14
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	8
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	0.0%	[-0.0%, 0.1%]	29

Bootstrap: 766.8s -> 766.847s (0.01%)
Artifact size: 362.14 MiB -> 362.26 MiB (0.03%)