Implement checked Shl/Shr at MIR building. #108282

cjgillot · 2023-02-20T19:35:45Z

This does not require any special handling by codegen backends,
as the overflow behaviour is entirely determined by the rhs (shift amount).

This allows MIR ConstProp to remove the overflow check for constant shifts.

There is an existing different behaviour between cg_llvm and cg_clif (cc @bjorn3).
I took cg_llvm's one as reference: overflow if rhs < 0 || rhs > number_of_bits_in_lhs_ty.

EDIT: cg_llvm and cg_clif implement the overflow check differently. This PR uses cg_llvm's implementation based on a BitAnd instead of cg_clif's one based on an unsigned comparison.

rustbot · 2023-02-20T19:35:51Z

r? @jackh726

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

rustbot · 2023-02-20T19:35:53Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

bjorn3 · 2023-02-20T19:49:04Z

There is an existing different behaviour between cg_llvm and cg_clif (cc @bjorn3).
I took cg_llvm's one as reference: overflow if rhs < 0 || rhs > number_of_bits_in_lhs_ty.

What is the difference? rhs < 0 is implied by rhs > number_of_bits_in_lhs_ty due to the usage of an unsigned comparison rather than a signed one.

tmiasko · 2023-02-21T17:59:52Z

Can you also update the docs for CheckedBinaryOp:

rust/compiler/rustc_middle/src/mir/syntax.rs

Lines 1115 to 1117 in 3200982

    
           /// For shift operations on integers the error condition is set when the value of right-hand 
        
           /// side is greater than or equal to the number of bits in the type of the left-hand side, or 
        
           /// when the value of right-hand side is negative.

rustbot · 2023-02-21T18:53:41Z

This PR changes MIR

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

cjgillot · 2023-02-22T20:14:17Z

@bors try @rust-timer queue

bors · 2023-02-22T20:14:27Z

⌛ Trying commit c0defbb19ecd3c083941ba2f7d7c050d83c0fa54 with merge b7075a68bbabaad51613d13371e9b12fc9da1d3b...

bors · 2023-02-22T23:04:22Z

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

rust-timer · 2023-02-23T04:49:28Z

Finished benchmarking commit (b7075a68bbabaad51613d13371e9b12fc9da1d3b): 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)	-	-	0
Regressions ❌ (secondary)	1.1%	[1.0%, 1.1%]	2
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.2%]	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
Regressions ❌ (secondary)	4.1%	[2.9%, 5.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.0%	[-5.4%, -2.6%]	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)	1.7%	[1.1%, 2.5%]	5
Regressions ❌ (secondary)	4.7%	[2.9%, 7.2%]	10
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.7%	[-6.2%, -3.8%]	4
All ❌✅ (primary)	1.7%	[1.1%, 2.5%]	5

RalfJung · 2023-03-12T16:56:01Z

Does this mean BinOp::is_checkable should no longer consider Shl/Shr to be checkable? Sounds like it to me.

cjgillot · 2023-03-13T16:10:55Z

I don't think so.

BinOp::is_checkable is poorly named: it controls whether -Coverflow-checks emits an assertion. This is still true: we still decide to emit/omit the assert terminator. #109058 makes the documentation more precise.

tmiasko · 2023-03-14T20:16:46Z

compiler/rustc_middle/src/ty/util.rs

+        let (int, signed) = match *self.kind() {
+            ty::Int(ity) => (Integer::from_int_ty(&tcx, ity), true),
+            ty::Uint(uty) => (Integer::from_uint_ty(&tcx, uty), false),
+            _ => bug!("non integer discriminant"),


nit: update the error message

tmiasko · 2023-03-14T20:18:13Z

@bors r+

bors · 2023-03-14T20:18:16Z

📌 Commit 0422f44 has been approved by tmiasko

It is now in the queue for this repository.

bors · 2023-03-14T20:59:48Z

⌛ Testing commit 0422f44 with merge d529528cedcad258a0e595aa0da70888b7d05bfc...

bors · 2023-03-15T01:14:48Z

💥 Test timed out

rust-log-analyzer · 2023-03-15T04:25:20Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

cjgillot · 2023-03-15T16:31:22Z

@bors retry

bors · 2023-03-15T21:31:10Z

⌛ Testing commit 0422f44 with merge c90eb48...

bors · 2023-03-16T01:01:40Z

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing c90eb48 to master...

rust-timer · 2023-03-17T07:35:07Z

Finished benchmarking commit (c90eb48): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.3%	[1.3%, 1.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.9%	[-1.3%, -0.5%]	8
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)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-2.9%	[-3.6%, -2.1%]	2
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Cycles

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

nnethercote · 2023-03-19T05:21:02Z

The tt-muncher improvements might be noise. The ctfe-stress-5 regressions are small and not really important. Looking at non-relevant (sub-threshold) results it might be a very slight win overall. In short: nothing worth worrying about.

@rustbot label: +perf-regression-triaged

Implement checked Shl/Shr at MIR building. This does not require any special handling by codegen backends, as the overflow behaviour is entirely determined by the rhs (shift amount). This allows MIR ConstProp to remove the overflow check for constant shifts. ~There is an existing different behaviour between cg_llvm and cg_clif (cc `@bjorn3).` I took cg_llvm's one as reference: overflow if `rhs < 0 || rhs > number_of_bits_in_lhs_ty`.~ EDIT: `cg_llvm` and `cg_clif` implement the overflow check differently. This PR uses `cg_llvm`'s implementation based on a `BitAnd` instead of `cg_clif`'s one based on an unsigned comparison.

Remove CBMC failure expectations that are now caught by the rust overflow check. This was due to the following Rust compiler change: - rust-lang/rust#108282

rustbot assigned jackh726 Feb 20, 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 Feb 20, 2023

cjgillot force-pushed the mir-checked-sh branch from 09fd064 to cc59862 Compare February 20, 2023 19:36

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 23, 2023

cjgillot added 2 commits February 27, 2023 19:25

Implement checked Shl/Shr at MIR building.

dd582bd

Update MIR doc.

0422f44

cjgillot force-pushed the mir-checked-sh branch from c0defbb to 0422f44 Compare February 27, 2023 19:34

RalfJung mentioned this pull request Mar 14, 2023

Document BinOp::is_checkable #109058

Merged

tmiasko reviewed Mar 14, 2023

View reviewed changes

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 Mar 14, 2023

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 Mar 15, 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 Mar 15, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 16, 2023

bors merged commit c90eb48 into rust-lang:master Mar 16, 2023

rustbot added this to the 1.70.0 milestone Mar 16, 2023

bors mentioned this pull request Mar 16, 2023

Add a context for making things #109186

Closed

3 tasks

cjgillot deleted the mir-checked-sh branch March 16, 2023 10:14

rustbot added the perf-regression Performance regression. label Mar 17, 2023

rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 19, 2023

celinval added a commit to celinval/kani-dev that referenced this pull request May 1, 2023

Change tests expectations

0033a60

Remove CBMC failure expectations that are now caught by the rust overflow check. This was due to the following Rust compiler change: - rust-lang/rust#108282

celinval mentioned this pull request May 1, 2023

Upgrade the toolchain to nightly-2023-04-16 model-checking/kani#2406

Merged

4 tasks

This was referenced Dec 21, 2023

arithmetic_overflow lint not triggered for overflowing shifts in promoteds #117949

Closed

Start emitting overflow lints for const promoted SHL & SHRs #119339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement checked Shl/Shr at MIR building. #108282

Implement checked Shl/Shr at MIR building. #108282

cjgillot commented Feb 20, 2023 •

edited

Loading

rustbot commented Feb 20, 2023

rustbot commented Feb 20, 2023

bjorn3 commented Feb 20, 2023

tmiasko commented Feb 21, 2023

rustbot commented Feb 21, 2023

cjgillot commented Feb 22, 2023

This comment has been minimized.

bors commented Feb 22, 2023

bors commented Feb 22, 2023

This comment has been minimized.

rust-timer commented Feb 23, 2023

RalfJung commented Mar 12, 2023

cjgillot commented Mar 13, 2023

tmiasko Mar 14, 2023

tmiasko commented Mar 14, 2023

bors commented Mar 14, 2023

bors commented Mar 14, 2023

bors commented Mar 15, 2023

rust-log-analyzer commented Mar 15, 2023

cjgillot commented Mar 15, 2023

bors commented Mar 15, 2023

bors commented Mar 16, 2023

rust-timer commented Mar 17, 2023

nnethercote commented Mar 19, 2023

Implement checked Shl/Shr at MIR building. #108282

Implement checked Shl/Shr at MIR building. #108282

Conversation

cjgillot commented Feb 20, 2023 • edited Loading

rustbot commented Feb 20, 2023

rustbot commented Feb 20, 2023

bjorn3 commented Feb 20, 2023

tmiasko commented Feb 21, 2023

rustbot commented Feb 21, 2023

cjgillot commented Feb 22, 2023

This comment has been minimized.

bors commented Feb 22, 2023

bors commented Feb 22, 2023

This comment has been minimized.

rust-timer commented Feb 23, 2023

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

Instruction count

Max RSS (memory usage)

Cycles

RalfJung commented Mar 12, 2023

cjgillot commented Mar 13, 2023

tmiasko Mar 14, 2023

Choose a reason for hiding this comment

tmiasko commented Mar 14, 2023

bors commented Mar 14, 2023

bors commented Mar 14, 2023

bors commented Mar 15, 2023

rust-log-analyzer commented Mar 15, 2023

cjgillot commented Mar 15, 2023

bors commented Mar 15, 2023

bors commented Mar 16, 2023

rust-timer commented Mar 17, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

nnethercote commented Mar 19, 2023

cjgillot commented Feb 20, 2023 •

edited

Loading