Promote unchecked integer math to MIR `BinOp`s #112238

scottmcm · 2023-06-03T10:56:54Z

So slice indexing by a range gets down to one basic block, for example.

r? cjgillot

rustbot · 2023-06-03T10:57:01Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes Stable MIR

cc @oli-obk, @celinval

Some changes occurred in rustc_ty_utils::consts.rs

cc @BoxyUwU

cjgillot · 2023-06-03T13:12:31Z

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

-        | sym::exact_div
-        | sym::unchecked_shl
-        | sym::unchecked_shr => {
+        sym::exact_div => {


Should exact_div be promoted too, or is it too rare to bother?

It's pretty rare on its own: looks like it's in the middle of align_offset, which is already pretty complicated, and as_chunks_unchecked, which is unstable, only.

The place it comes in where it's really important is in sub_ptr, but that's its own intrinsic, so the exact_div doesn't show up in MIR at all.

So I think I'd like to leave it out of this PR, so it stays focused on just the "overflow is UB" ones.

cjgillot · 2023-06-03T13:17:04Z

compiler/rustc_const_eval/src/transform/validate.rs

@@ -601,6 +601,24 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                            );
                        }
                    }
+                    AddUnchecked | SubUnchecked | MulUnchecked => {


Can this reuse the previous branch?

I split it out because the *Unchecked ones don't work on floats, which the previous branch accepts.

I pulled out the "equal types" checks to a shared check, and used that to merge some of the arms.

@rustbot ready

RalfJung

Interpreter changes look good!

add unchecked_shl test rust-lang/rust#112238 made me realize that we have a test for add,sub,mul,shr but not shl. Add the missing test. Also name the existing tests more consistently.

add unchecked_shl test rust-lang#112238 made me realize that we have a test for add,sub,mul,shr but not shl. Add the missing test. Also name the existing tests more consistently.

scottmcm · 2023-06-03T18:24:20Z

Thanks, Ralf! The MIRI tests were very helpful in ensuring I got everything exactly the same here 🙂

cjgillot · 2023-06-09T16:26:57Z

@bors r+

bors · 2023-06-09T16:26:59Z

📌 Commit 33be521 has been approved by cjgillot

It is now in the queue for this repository.

…llot Promote unchecked integer math to MIR `BinOp`s So slice indexing by a range gets down to one basic block, for example. r? cjgillot

bors · 2023-06-10T00:10:28Z

⌛ Testing commit 33be521 with merge 26d6ddd3f0aa50c1224ac38fdac9bb15231829e0...

bors · 2023-06-10T02:16:17Z

💔 Test failed - checks-actions

bors · 2023-06-17T03:24:01Z

⌛ Trying commit 015f6ec719934c6082eabdd203b006fcdeec2c56 with merge d2a3397004f36e2ad1a270be102c6637ae511629...

bors · 2023-06-17T04:41:42Z

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

rust-timer · 2023-06-17T05:57:14Z

Finished benchmarking commit (d2a3397004f36e2ad1a270be102c6637ae511629): comparison URL.

Overall result: ❌✅ regressions and improvements - 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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[1.0%, 1.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.8%, 1.0%]	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)	5.1%	[5.1%, 5.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	5.1%	[5.1%, 5.1%]	1

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

Binary size

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.1%	[0.0%, 0.5%]	33
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-0.1%	[-0.3%, -0.0%]	10
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	0.1%	[-0.3%, 0.5%]	43

Bootstrap: 656.882s -> 655.76s (-0.17%)

cjgillot · 2023-06-19T15:32:56Z

@bors r+

bors · 2023-06-19T15:32:58Z

📌 Commit c780e55 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-06-19T17:14:24Z

⌛ Testing commit c780e55 with merge 4051305...

bors · 2023-06-19T20:01:44Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 4051305 to master...

scottmcm · 2023-06-19T20:04:06Z

Hooray, #112673 worked, then 🎉

rust-timer · 2023-06-19T21:16:07Z

Finished benchmarking commit (4051305): comparison URL.

Overall result: ❌ regressions - 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)	1.6%	[1.6%, 1.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[1.6%, 1.6%]	1

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)	3.0%	[2.1%, 3.8%]	3
Regressions ❌ (secondary)	2.7%	[2.6%, 2.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[2.1%, 3.8%]	3

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)	1.7%	[1.7%, 1.7%]	1
Regressions ❌ (secondary)	7.5%	[1.3%, 12.8%]	11
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.7%	[1.7%, 1.7%]	1

Binary size

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

Bootstrap: 656.783s -> 658.381s (0.24%)

Patches: * rust-lang/rust#112119 * rust-lang/rust#108293 (partially) * rust-lang/rust#112786 * rust-lang/rust#112060 * rust-lang/rust#112163 * rust-lang/rust#112238 * rust-lang/rust#112165

rustbot assigned cjgillot Jun 3, 2023

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 Jun 3, 2023

cjgillot reviewed Jun 3, 2023

View reviewed changes

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2023

RalfJung approved these changes Jun 3, 2023

View reviewed changes

RalfJung mentioned this pull request Jun 3, 2023

add unchecked_shl test rust-lang/miri#2914

Merged

scottmcm force-pushed the mir-add-unchecked branch from 739af0e to b96de9e Compare June 4, 2023 07:10

scottmcm mentioned this pull request Jun 4, 2023

Use MIR's Offset for pointer add too #110837

Merged

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2023

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 Jun 9, 2023

GuillaumeGomez mentioned this pull request Jun 9, 2023

Rollup of 4 pull requests #112471

Closed

This comment has been minimized.

Sign in to view

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

scottmcm force-pushed the mir-add-unchecked branch from 33be521 to adf1624 Compare June 10, 2023 05:08

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 17, 2023

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jun 17, 2023

scottmcm added 3 commits June 19, 2023 01:47

Promote unchecked_add/sub/mul/shl/shr to mir::BinOp

39788e0

Remove unchecked_add/sub/mul/shl/shr from CTFE/cg_ssa/cg_clif

3fd8501

Dedup some type checks in the MIR validator

c780e55

scottmcm force-pushed the mir-add-unchecked branch from 015f6ec to c780e55 Compare June 19, 2023 09:00

scottmcm marked this pull request as ready for review June 19, 2023 09:04

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 19, 2023

bors merged commit 4051305 into rust-lang:master Jun 19, 2023

rustbot added this to the 1.72.0 milestone Jun 19, 2023

This was referenced Jun 19, 2023

Implement a real InstCombine MIR pass #105808

Closed

Enable MIR reference propagation by default #109025

Merged

Perform opportunistic simplifications during value numbering #111344

Closed

scottmcm deleted the mir-add-unchecked branch June 20, 2023 01:46

celinval mentioned this pull request Jun 21, 2023

Upgrade rust toolchain to nightly-2023-06-20 model-checking/kani#2551

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promote unchecked integer math to MIR `BinOp`s #112238

Promote unchecked integer math to MIR `BinOp`s #112238

scottmcm commented Jun 3, 2023

rustbot commented Jun 3, 2023

cjgillot Jun 3, 2023

scottmcm Jun 3, 2023

cjgillot Jun 3, 2023

scottmcm Jun 3, 2023

scottmcm Jun 4, 2023

RalfJung left a comment

scottmcm commented Jun 3, 2023

cjgillot commented Jun 9, 2023

bors commented Jun 9, 2023

bors commented Jun 10, 2023

This comment has been minimized.

bors commented Jun 10, 2023

bors commented Jun 17, 2023

bors commented Jun 17, 2023

This comment has been minimized.

rust-timer commented Jun 17, 2023

cjgillot commented Jun 19, 2023

bors commented Jun 19, 2023

bors commented Jun 19, 2023

bors commented Jun 19, 2023

scottmcm commented Jun 19, 2023

rust-timer commented Jun 19, 2023

Promote unchecked integer math to MIR BinOps #112238

Promote unchecked integer math to MIR BinOps #112238

Conversation

scottmcm commented Jun 3, 2023

rustbot commented Jun 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung left a comment

Choose a reason for hiding this comment

scottmcm commented Jun 3, 2023

cjgillot commented Jun 9, 2023

bors commented Jun 9, 2023

bors commented Jun 10, 2023

This comment has been minimized.

bors commented Jun 10, 2023

bors commented Jun 17, 2023

bors commented Jun 17, 2023

This comment has been minimized.

rust-timer commented Jun 17, 2023

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

cjgillot commented Jun 19, 2023

bors commented Jun 19, 2023

bors commented Jun 19, 2023

bors commented Jun 19, 2023

scottmcm commented Jun 19, 2023

rust-timer commented Jun 19, 2023

Overall result: ❌ regressions - no action needed

Promote unchecked integer math to MIR `BinOp`s #112238

Promote unchecked integer math to MIR `BinOp`s #112238