interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch #117832

RalfJung · 2023-11-12T11:19:37Z

While we're at it, also update comments in codegen and MIR building related to shifts, and fix the overflow error printed by Miri on negative shift amounts.

rustbot · 2023-11-12T11:19:45Z

r? @cjgillot

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

rustbot · 2023-11-12T11:19:47Z

This PR changes MIR

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

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2023-11-12T11:21:47Z

compiler/rustc_mir_build/src/build/expr/as_rvalue.rs

@@ -600,10 +600,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
            BinOp::Shl | BinOp::Shr if self.check_overflow && ty.is_integral() => {
                // For an unsigned RHS, the shift is in-range for `rhs < bits`.
                // For a signed RHS, `IntToInt` cast to the equivalent unsigned
-                // type and do that same comparison. Because the type is the
-                // same size, there's no negative shift amount that ends up
-                // overlapping with valid ones, thus it catches negatives too.


@scottmcm you wrote in b537e6b "because the type is the same size, there's no negative shift amount that ends up overlapping with valid ones". I don't understand that argument. What does "we're casting from signed to unsigned with the same size" have to do with the second half of the sentence?

I have replaced this comment with an argument that I do understand.

I don't remember what I meant by "same size", since mixed-width shifts are allowed for MIR.

What you have now -- that iN::MIN as uN will never conflict with a legal shift -- is really what I was trying to express, I think.

…signed and unsigned shift amounts in the same branch

cjgillot · 2023-11-19T20:32:58Z

@bors r+

bors · 2023-11-19T20:33:00Z

📌 Commit 31493c7 has been approved by cjgillot

It is now in the queue for this repository.

interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch While we're at it, also update comments in codegen and MIR building related to shifts, and fix the overflow error printed by Miri on negative shift amounts.

…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#115526 (Add arm64e-apple-ios & arm64e-apple-darwin targets) - rust-lang#115691 (Add `$message_type` field to distinguish json diagnostic outputs) - rust-lang#117828 (Avoid iterating over hashmaps in astconv) - rust-lang#117832 (interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch) - rust-lang#117891 (Recover `dyn` and `impl` after `for<...>`) - rust-lang#117957 (if available use a Child's pidfd for kill/wait) - rust-lang#117994 (Ignore but do not assume region obligations from unifying headers in negative coherence) - rust-lang#118068 (subtree update cg_gcc 2023/11/17) r? `@ghost` `@rustbot` modify labels: rollup

…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#117828 (Avoid iterating over hashmaps in astconv) - rust-lang#117832 (interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch) - rust-lang#117891 (Recover `dyn` and `impl` after `for<...>`) - rust-lang#117957 (if available use a Child's pidfd for kill/wait) - rust-lang#117988 (Handle attempts to have multiple `cfg`d tail expressions) - rust-lang#117994 (Ignore but do not assume region obligations from unifying headers in negative coherence) - rust-lang#118000 (Make regionck care about placeholders in outlives components) - rust-lang#118068 (subtree update cg_gcc 2023/11/17) r? `@ghost` `@rustbot` modify labels: rollup

…tthiaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#117828 (Avoid iterating over hashmaps in astconv) - rust-lang#117832 (interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch) - rust-lang#117891 (Recover `dyn` and `impl` after `for<...>`) - rust-lang#117957 (if available use a Child's pidfd for kill/wait) - rust-lang#117988 (Handle attempts to have multiple `cfg`d tail expressions) - rust-lang#117994 (Ignore but do not assume region obligations from unifying headers in negative coherence) - rust-lang#118000 (Make regionck care about placeholders in outlives components) - rust-lang#118068 (subtree update cg_gcc 2023/11/17) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#117832 - RalfJung:interpret-shift, r=cjgillot interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch While we're at it, also update comments in codegen and MIR building related to shifts, and fix the overflow error printed by Miri on negative shift amounts.

bors · 2023-11-20T13:27:49Z

⌛ Testing commit 31493c7 with merge 46ecc10...

Noratrieb · 2023-11-20T17:25:54Z

https://bors.rust-lang.org/queue/rust according to the queue, this is currently being bored...
@bors r- wtf are you doing

Noratrieb · 2023-11-20T17:26:21Z

RalfJung · 2023-11-20T17:31:03Z

@bors force

dtolnay · 2023-11-20T18:54:15Z

5 hours later, still running in queue and blocking other PRs from being tested.

I believe this interrupts a currently running build:

@bors retry

dtolnay · 2023-11-20T18:58:22Z

That worked — this PR is no longer "pending" at the top of the queue.

But no other PR has immediately started being tested, as far as I can tell, which is a bad sign.

dtolnay · 2023-11-20T19:01:49Z

Okay it just took 6 minutes after "bors retry", but #118093 (comment) is testing now. Everything looks back to normal.

rustbot assigned cjgillot Nov 12, 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 Nov 12, 2023

RalfJung commented Nov 12, 2023

View reviewed changes

RalfJung force-pushed the interpret-shift branch from 5bc4a16 to efa6ebf Compare November 12, 2023 11:23

RalfJung mentioned this pull request Nov 12, 2023

Inconsistency in behavior of MIR and SIMD shifts #91237

Open

RalfJung force-pushed the interpret-shift branch from efa6ebf to 1ee7f36 Compare November 12, 2023 11:28

This comment has been minimized.

Sign in to view

interpret: simplify handling of shifts by no longer trying to handle …

31493c7

…signed and unsigned shift amounts in the same branch

RalfJung force-pushed the interpret-shift branch from 1ee7f36 to 31493c7 Compare November 12, 2023 11:49

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 Nov 19, 2023

compiler-errors mentioned this pull request Nov 20, 2023

Rollup of 8 pull requests #118078

Closed

compiler-errors mentioned this pull request Nov 20, 2023

Rollup of 8 pull requests #118082

Merged

bors merged commit 94d9b7e into rust-lang:master Nov 20, 2023
11 checks passed

rustbot added this to the 1.76.0 milestone Nov 20, 2023

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 20, 2023

RalfJung deleted the interpret-shift branch November 20, 2023 17:30

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 Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch #117832

interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch #117832

RalfJung commented Nov 12, 2023

rustbot commented Nov 12, 2023

rustbot commented Nov 12, 2023

RalfJung Nov 12, 2023

scottmcm Nov 20, 2023

This comment has been minimized.

cjgillot commented Nov 19, 2023

bors commented Nov 19, 2023

bors commented Nov 20, 2023

Noratrieb commented Nov 20, 2023

Noratrieb commented Nov 20, 2023

RalfJung commented Nov 20, 2023

dtolnay commented Nov 20, 2023

dtolnay commented Nov 20, 2023

dtolnay commented Nov 20, 2023

interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch #117832

interpret: simplify handling of shifts by no longer trying to handle signed and unsigned shift amounts in the same branch #117832

Conversation

RalfJung commented Nov 12, 2023

rustbot commented Nov 12, 2023

rustbot commented Nov 12, 2023

RalfJung Nov 12, 2023

Choose a reason for hiding this comment

scottmcm Nov 20, 2023

Choose a reason for hiding this comment

This comment has been minimized.

cjgillot commented Nov 19, 2023

bors commented Nov 19, 2023

bors commented Nov 20, 2023

Noratrieb commented Nov 20, 2023

Noratrieb commented Nov 20, 2023

RalfJung commented Nov 20, 2023

dtolnay commented Nov 20, 2023

dtolnay commented Nov 20, 2023

dtolnay commented Nov 20, 2023