interpret: make overflowing binops just normal binops #125359

RalfJung · 2024-05-21T11:48:36Z

rustbot · 2024-05-21T11:48:45Z

rustbot has assigned @compiler-errors.
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

rustbot · 2024-05-21T11:48:46Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2024-05-21T11:49:37Z

compiler/rustc_mir_transform/src/gvn.rs

@@ -223,7 +223,7 @@ enum Value<'tcx> {
 NullaryOp(NullOp<'tcx>, Ty<'tcx>),
 UnaryOp(UnOp, VnIndex),
 BinaryOp(BinOp, VnIndex, VnIndex),
- CheckedBinaryOp(BinOp, VnIndex, VnIndex),
+ CheckedBinaryOp(BinOp, VnIndex, VnIndex), // FIXME get rid of this, work like MIR instead


I think there's more clean-up possible here (and in dataflow_const_prop as well), but I'll leave that to someone else.

Cc @cjgillot

RalfJung · 2024-05-21T11:52:27Z

compiler/rustc_const_eval/src/interpret/operator.rs

- let val_field = self.project_field(dest, 0)?;
- self.write_scalar(val.to_scalar(), &val_field)?;
- let overflowed_field = self.project_field(dest, 1)?;
- self.write_scalar(Scalar::from_bool(overflowed), &overflowed_field)?;


Removing this code means we now assume that the return type of the WithOverflow ops actually is a ScalarPair. I think we should enforce that our primitive operations return data we can represent with out primitive types, so IMO this is a reasonable assumption. At some point we added the codepath here to account for layout randomization, but looking through the layout code I think tuples are using ReprFlags::default() which will not have RANDOMIZE_LAYOUT set so this shouldn't happen any more?

If we ever make it happen again, we should not randomize tuples of the form ($int, bool).

Looking at codegen I can't see support there for checked-binops returning something that does not have the ScalarPair ABI.

EDIT: Ah, that was in preparation for #97861 which never landed. Seems fine to un-do then, IMO.

👍 to saying it has to be a scalar pair. We could, potentially, still allow layout randomization on it within that constraint, however -- not that we should do that (at least right now), though.

I think we could add layout-aware scalar-pair constructors (and destructors) that work like

rust/compiler/rustc_codegen_ssa/src/mir/rvalue.rs

Lines 713 to 724 in 54cdc13

let mut inputs = ArrayVec::<Bx::Value, 2>::new();

let mut input_scalars = ArrayVec::<abi::Scalar, 2>::new();

for field_idx in layout.fields.index_by_increasing_offset() {

let field_idx = FieldIdx::from_usize(field_idx);

let op = self.codegen_operand(bx, &fields[field_idx]);

let values = op.val.immediates_or_place().left_or_else(|p| {

bug!("Field {field_idx:?} is {p:?} making {layout:?}");

});

inputs.extend(values);

let scalars = self.value_kind(op.layout).scalars().unwrap();

input_scalars.extend(scalars);

}

if we needed to.

But yeah, no need to do that for now, and the way we do layout optimizations for structs we put bigger, higher-alignment types first anyway, so (T, bool) will I think always be in that order anyway right now.

(Or we could add a repr(linear) type for WithOverflow<T> that these could return, if needed, though that seems like overkill.)

(To elaborate on this, one thing that cg_ssa does now is that the in-register type doesn't need to be an exact match for the in-memory type. So we could -- potentially and in the future -- do something like allow the 8 possible randomizations of (u32, bool) while still saying that it's a scalar pair.)

That's for Rvalue::Aggregate, I don't see how it is related.

But yeah we could make ScalarPair more flexible, we just need to add the information at which offset the two fields are living. Then all variants of ($int, bool) can be represented. (Also see here and here for the more general topic of refactoring our Abi handling.)

RalfJung · 2024-05-21T12:48:28Z

compiler/rustc_mir_transform/src/dataflow_const_prop.rs

 // Overflows must be ignored here.
+ // The overflowing operators are handled in `handle_assign`.


I don't understand why the overflowing operators can't be handled here, but it matches the previous structure where Rvalue::CheckedBinaryOp was handled in handle_assign, not in handle_rvalue, for some reason.

As checked ops return scalar pairs, the routine needs to project the destination as 2 scalars, which handle_rvalue is not designed to do.

Would be good to add a comment in the code explaining that.

scottmcm · 2024-05-22T05:47:11Z

compiler/rustc_const_eval/src/interpret/intrinsics.rs

+ self.binary_op(BinOp::SubWithOverflow, &a_offset, &b_offset)?
+ .to_scalar_pair()


Oh, nice. I like the approach of it returning ImmTy so the method doesn't need to talk about tuples 👍

scottmcm

I did a (admittedly fairly quick) scan over this and it seems good to me, but you should probably get a review from someone more familiar with the nuances of CTFE/Miri.

compiler/rustc_const_eval/src/interpret/operator.rs

scottmcm · 2024-05-22T05:57:47Z

compiler/rustc_const_eval/src/interpret/operator.rs

- let (res, overflow) = match un_op {
- Not => (self.truncate(!val, layout), false), // bitwise negation, then truncate
+ let res = match un_op {
+ Not => self.truncate(!val, layout), // bitwise negation, then truncate
 Neg => {


nothing to do for this PR: Interesting that we don't have a checked version of neg in mir anywhere. I guess it's like Div in that lowering to MIR deals with that.

compiler/rustc_middle/src/mir/tcx.rs

Co-authored-by: scottmcm <[email protected]>

RalfJung · 2024-05-22T09:21:30Z

you should probably get a review from someone more familiar with the nuances of CTFE/Miri.

r? @oli-obk

oli-obk · 2024-05-22T10:08:16Z

@bors try @rust-timer queue

r=me with perf clean. I'll give the known panics pass another look for more cleanups once this lands

…r=<try> interpret: make overflowing binops just normal binops Follow-up to rust-lang#125173 (Cc `@scottmcm)`

bors · 2024-05-22T10:09:29Z

⌛ Trying commit cb53194 with merge d373d09...

bors · 2024-05-22T11:45:34Z

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

RalfJung · 2024-05-22T11:49:47Z

FWIW I think GVN/const-prop have the more obvious cleanup possibilities. But yeah known-lints can probably also be cleaned up a bit more.

rust-timer · 2024-05-22T17:01:19Z

Finished benchmarking commit (d373d09): 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 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.8%	[1.8%, 1.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

Results (primary -2.9%, secondary -2.1%)

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.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-2.1%	[-2.2%, -2.1%]	2
All ❌✅ (primary)	-2.9%	[-2.9%, -2.9%]	1

Binary size

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

Bootstrap: 672.666s -> 674.077s (0.21%)
Artifact size: 315.47 MiB -> 315.95 MiB (0.15%)

…piler-errors Stop using `to_hir_binop` in codegen This came up in rust-lang#125359 (comment) , and looking into it we can just use the `mir::BinOp`s directly instead of `hir::BinOpKind`s. (AKA rather than going `mir::BinOp` → `hir::BinOpKind` → `IntPredicate`, just go `mir::BinOp` → `IntPredicate`.)

RalfJung · 2024-05-22T17:15:28Z

Seems clean enough to me -- a barely significant regression that only affects some build configurations of the stress test.

scottmcm · 2024-05-22T17:20:36Z

Agreed, Ralf. The bot doesn't consider it a regression, and while cycles are unreliable, they're not showing an issue either.
@bors r=oli-obk

bors · 2024-05-22T17:20:39Z

📌 Commit cb53194 has been approved by oli-obk

It is now in the queue for this repository.

…piler-errors Stop using `to_hir_binop` in codegen This came up in rust-lang#125359 (comment) , and looking into it we can just use the `mir::BinOp`s directly instead of `hir::BinOpKind`s. (AKA rather than going `mir::BinOp` → `hir::BinOpKind` → `IntPredicate`, just go `mir::BinOp` → `IntPredicate`.)

Rollup merge of rust-lang#125399 - scottmcm:less-hir-in-cg_ssa, r=compiler-errors Stop using `to_hir_binop` in codegen This came up in rust-lang#125359 (comment) , and looking into it we can just use the `mir::BinOp`s directly instead of `hir::BinOpKind`s. (AKA rather than going `mir::BinOp` → `hir::BinOpKind` → `IntPredicate`, just go `mir::BinOp` → `IntPredicate`.)

bors · 2024-05-23T04:03:17Z

⌛ Testing commit cb53194 with merge 5293c6a...

Stop using `to_hir_binop` in codegen This came up in rust-lang/rust#125359 (comment) , and looking into it we can just use the `mir::BinOp`s directly instead of `hir::BinOpKind`s. (AKA rather than going `mir::BinOp` → `hir::BinOpKind` → `IntPredicate`, just go `mir::BinOp` → `IntPredicate`.)

bors · 2024-05-23T06:12:56Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 5293c6a to master...

rust-timer · 2024-05-23T07:33:04Z

Finished benchmarking commit (5293c6a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary -1.1%)

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)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Cycles

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

Binary size

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

Bootstrap: 674.126s -> 672.751s (-0.20%)
Artifact size: 315.70 MiB -> 315.57 MiB (-0.04%)

scottmcm · 2024-05-23T15:22:46Z

Ah, great -- no regressions post-merge so the two "highly reliable" ones pre-merge definitely weren't an issue.

rustbot assigned compiler-errors May 21, 2024

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 May 21, 2024

RalfJung commented May 21, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the interpret-overflowing-ops branch from f18d8a3 to aa3a9ea Compare May 21, 2024 12:47

RalfJung commented May 21, 2024

View reviewed changes

interpret: make overflowing binops just normal binops

c0b4b45

RalfJung force-pushed the interpret-overflowing-ops branch from aa3a9ea to c0b4b45 Compare May 21, 2024 12:50

scottmcm reviewed May 22, 2024

View reviewed changes

scottmcm approved these changes May 22, 2024

View reviewed changes

scottmcm mentioned this pull request May 22, 2024

Stop using to_hir_binop in codegen #125399

Merged

clarify comment

cb53194

Co-authored-by: scottmcm <[email protected]>

RalfJung force-pushed the interpret-overflowing-ops branch from d549576 to cb53194 Compare May 22, 2024 09:19

rustbot assigned oli-obk and unassigned compiler-errors May 22, 2024

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 May 22, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024

Auto merge of rust-lang#125359 - RalfJung:interpret-overflowing-ops, …

d373d09

…r=<try> interpret: make overflowing binops just normal binops Follow-up to rust-lang#125173 (Cc `@scottmcm)`

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 May 22, 2024

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 May 22, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label May 23, 2024

bors merged commit 5293c6a into rust-lang:master May 23, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone May 23, 2024

RalfJung deleted the interpret-overflowing-ops branch May 23, 2024 06:16

bors mentioned this pull request May 23, 2024

Remove more #[macro_use] extern crate tracing #125434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interpret: make overflowing binops just normal binops #125359

interpret: make overflowing binops just normal binops #125359

RalfJung commented May 21, 2024

rustbot commented May 21, 2024

rustbot commented May 21, 2024

RalfJung May 21, 2024

RalfJung May 21, 2024 •

edited

Loading

scottmcm May 22, 2024 •

edited

Loading

scottmcm May 22, 2024

RalfJung May 22, 2024

This comment has been minimized.

RalfJung May 21, 2024

cjgillot Jun 2, 2024

RalfJung Jun 2, 2024

scottmcm May 22, 2024

scottmcm left a comment

scottmcm May 22, 2024

RalfJung commented May 22, 2024

oli-obk commented May 22, 2024

This comment has been minimized.

bors commented May 22, 2024

bors commented May 22, 2024

This comment has been minimized.

RalfJung commented May 22, 2024

rust-timer commented May 22, 2024

RalfJung commented May 22, 2024

scottmcm commented May 22, 2024

bors commented May 22, 2024

bors commented May 23, 2024

bors commented May 23, 2024

rust-timer commented May 23, 2024

scottmcm commented May 23, 2024

	let mut inputs = ArrayVec::<Bx::Value, 2>::new();
	let mut input_scalars = ArrayVec::<abi::Scalar, 2>::new();
	for field_idx in layout.fields.index_by_increasing_offset() {
	let field_idx = FieldIdx::from_usize(field_idx);
	let op = self.codegen_operand(bx, &fields[field_idx]);
	let values = op.val.immediates_or_place().left_or_else(\|p\| {
	bug!("Field {field_idx:?} is {p:?} making {layout:?}");
	});
	inputs.extend(values);
	let scalars = self.value_kind(op.layout).scalars().unwrap();
	input_scalars.extend(scalars);
	}

		// Overflows must be ignored here.
		// The overflowing operators are handled in `handle_assign`.

		self.binary_op(BinOp::SubWithOverflow, &a_offset, &b_offset)?
		.to_scalar_pair()

interpret: make overflowing binops just normal binops #125359

interpret: make overflowing binops just normal binops #125359

Conversation

RalfJung commented May 21, 2024

rustbot commented May 21, 2024

rustbot commented May 21, 2024

Choose a reason for hiding this comment

RalfJung May 21, 2024 • edited Loading

Choose a reason for hiding this comment

scottmcm May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

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

scottmcm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented May 22, 2024

oli-obk commented May 22, 2024

This comment has been minimized.

bors commented May 22, 2024

bors commented May 22, 2024

This comment has been minimized.

RalfJung commented May 22, 2024

rust-timer commented May 22, 2024

Overall result: ❌ regressions - no action needed

RalfJung commented May 22, 2024

scottmcm commented May 22, 2024

bors commented May 22, 2024

bors commented May 23, 2024

bors commented May 23, 2024

rust-timer commented May 23, 2024

Overall result: no relevant changes - no action needed

scottmcm commented May 23, 2024

RalfJung May 21, 2024 •

edited

Loading

scottmcm May 22, 2024 •

edited

Loading