Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promote unchecked integer math to MIR BinOps #112238

Merged
merged 3 commits into from
Jun 19, 2023

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Jun 3, 2023

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

r? cjgillot

@rustbot 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
@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2023

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

| sym::exact_div
| sym::unchecked_shl
| sym::unchecked_shr => {
sym::exact_div => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this reuse the previous branch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rustbot ready

@cjgillot 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
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interpreter changes look good!

bors added a commit to rust-lang/miri that referenced this pull request Jun 3, 2023
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.
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Jun 3, 2023
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
Copy link
Member Author

scottmcm commented Jun 3, 2023

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

@rustbot 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
@cjgillot
Copy link
Contributor

cjgillot commented Jun 9, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2023

📌 Commit 33be521 has been approved by cjgillot

It is now in the queue for this repository.

@bors 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 added a commit to GuillaumeGomez/rust that referenced this pull request Jun 9, 2023
…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
Copy link
Contributor

bors commented Jun 10, 2023

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

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 10, 2023

💔 Test failed - checks-actions

@bors 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
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 17, 2023
@bors
Copy link
Contributor

bors commented Jun 17, 2023

⌛ Trying commit 015f6ec719934c6082eabdd203b006fcdeec2c56 with merge d2a3397004f36e2ad1a270be102c6637ae511629...

@bors
Copy link
Contributor

bors commented Jun 17, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

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%)

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jun 17, 2023
@scottmcm scottmcm marked this pull request as ready for review June 19, 2023 09:04
@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 19, 2023

📌 Commit c780e55 has been approved by cjgillot

It is now in the queue for this repository.

@bors 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
Copy link
Contributor

bors commented Jun 19, 2023

⌛ Testing commit c780e55 with merge 4051305...

@bors
Copy link
Contributor

bors commented Jun 19, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 19, 2023
@bors bors merged commit 4051305 into rust-lang:master Jun 19, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 19, 2023
@scottmcm
Copy link
Member Author

Hooray, #112673 worked, then 🎉

@rust-timer
Copy link
Collaborator

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%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants