interpret: don't rely on ScalarPair for overflowed arithmetic by RalfJung · Pull Request #98627 · rust-lang/rust

RalfJung · 2022-06-28T15:41:39Z

I would like to avoid making this depend on dest.layout.abi to avoid a branch that we are not usually covering both sides of. Though OTOH this seems like fairly straight-forward code. But let's benchmark this option first to see how bad that extra force_allocation really is.

rustbot · 2022-06-28T15:41:43Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2022-06-28T15:41:43Z

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

RalfJung · 2022-06-28T15:41:57Z

@bors try @rust-timer queue

rust-timer · 2022-06-28T15:41:59Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-06-28T15:42:06Z

⌛ Trying commit a863894797f329841331df301bdaf275c0e3ff94 with merge 57aca588d42161a2fd53f97a85fc4eb9f62f8e38...

bors · 2022-06-28T17:16:39Z

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

rust-timer · 2022-06-28T17:16:42Z

Queued 57aca588d42161a2fd53f97a85fc4eb9f62f8e38 with parent baf382e, future comparison URL.

RalfJung · 2022-06-28T17:30:07Z

That is strange, is this making const-prop worse?

rust-timer · 2022-06-28T18:45:19Z

Finished benchmarking commit (57aca588d42161a2fd53f97a85fc4eb9f62f8e38): comparison url.

Instruction count

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	0.2%	0.2%	1
Regressions 😿 (secondary)	7.7%	8.8%	7
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-0.6%	-0.7%	6
All 😿🎉 (primary)	0.2%	0.2%	1

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	0.3%	0.3%	1
Regressions 😿 (secondary)	3.0%	3.0%	1
Improvements 🎉 (primary)	-1.9%	-1.9%	1
Improvements 🎉 (secondary)	-2.3%	-2.3%	1
All 😿🎉 (primary)	-0.8%	-1.9%	2

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	8.6%	11.6%	7
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

bjorn3 · 2022-06-28T18:56:38Z

Cg_clif makes the same assumption with respect to the ScalarPair ABI being used for CheckedBinOp.

RalfJung · 2022-06-28T19:12:47Z

As expected, the CTFE benchmark is not happy.

We probably should go with @eddyb's proposal of checking the dest.layout.abi then... but maybe also fall back to the slow path in debug builds, just to ensure it remains covered? OTOH then maybe we don't cover the fast path enough.^^

oli-obk · 2022-06-28T20:03:59Z

Oh yea this affects const prop. Const prop likely doesn't handle locals backed by an alloc properly and optimizes/lints less

RalfJung · 2022-06-28T21:05:29Z

All right, I brought back the old code path. It is not used in debug builds.

RalfJung · 2022-06-28T22:38:17Z

Aand I guess CI is using a debug build. Argh.
Is there the equivalent of // ignore-debug for run-make tests?

RalfJung · 2022-07-03T13:57:08Z

Well, this should fix CI. The only problem is that now the new code path is fully untested...

lcnr · 2022-07-04T08:09:42Z

i am fine with that

r=me unless you want to try something else

lcnr · 2022-07-04T08:10:14Z

+            let pair = Immediate::ScalarPair(val.into(), Scalar::from_bool(overflowed).into());
+            self.write_immediate(pair, dest)?;
+        } else {
+            // With randomized layout, `(int, bool)` might cease to be a `ScalarPair`, so we have to


maybe explicitly assert that we're using -Zrandomize-layout (or however the flag is called) here?

RalfJung · 2022-07-04T13:19:37Z

@bors r=lcnr

@Kixiron I hope you'll make this covered by tests soon? :D

bors · 2022-07-04T13:19:38Z

📌 Commit 0850bad has been approved by lcnr

Kixiron · 2022-07-04T13:22:54Z

Sure, I can totally try to test layout randomization in a later PR

bors · 2022-07-04T20:00:44Z

⌛ Testing commit 0850bad with merge 27eb6d7...

bors · 2022-07-04T22:42:10Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 27eb6d7 to master...

rust-timer · 2022-07-05T01:59:28Z

Finished benchmarking commit (27eb6d7): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.0%	2.0%	1
Improvements 🎉 (primary)	-1.5%	-1.5%	1
Improvements 🎉 (secondary)	-4.7%	-4.7%	1
All 😿🎉 (primary)	-1.5%	-1.5%	1

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	3.0%	3.0%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	3.0%	3.0%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

rust-highfive assigned lcnr Jun 28, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 28, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2022

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

RalfJung mentioned this pull request Jun 28, 2022

Added randomized field padding to -Z randomize-layout #97861

Closed

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 28, 2022

RalfJung force-pushed the interpret-arith branch from 129fe71 to 9eabc3b Compare June 28, 2022 21:04

This comment has been minimized.

Sign in to view

interpret: don't rely on ScalarPair for overflowed arithmetic

595dd97

RalfJung force-pushed the interpret-arith branch from 9eabc3b to 595dd97 Compare July 3, 2022 13:56

lcnr reviewed Jul 4, 2022

View reviewed changes

extra assertion, extra sure

0850bad

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 Jul 4, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 4, 2022

bors merged commit 27eb6d7 into rust-lang:master Jul 4, 2022

rustbot added this to the 1.64.0 milestone Jul 4, 2022

bors mentioned this pull request Jul 4, 2022

interpret: fix CheckedBinOp behavior when overflow checking is disabled #98888

Merged

rustbot removed the perf-regression Performance regression. label Jul 5, 2022

RalfJung deleted the interpret-arith branch July 6, 2022 02:27

Uh oh!

Conversation

RalfJung commented Jun 28, 2022

Uh oh!

rustbot commented Jun 28, 2022

Uh oh!

rust-highfive commented Jun 28, 2022

Uh oh!

RalfJung commented Jun 28, 2022

Uh oh!

rust-timer commented Jun 28, 2022

Uh oh!

bors commented Jun 28, 2022

Uh oh!

bors commented Jun 28, 2022

Uh oh!

rust-timer commented Jun 28, 2022

Uh oh!

RalfJung commented Jun 28, 2022

Uh oh!

rust-timer commented Jun 28, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

bjorn3 commented Jun 28, 2022

Uh oh!

RalfJung commented Jun 28, 2022

Uh oh!

oli-obk commented Jun 28, 2022

Uh oh!

RalfJung commented Jun 28, 2022

Uh oh!

RalfJung commented Jun 28, 2022

Uh oh!

This comment has been minimized.

RalfJung commented Jul 3, 2022

Uh oh!

lcnr commented Jul 4, 2022

Uh oh!

lcnr Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Jul 4, 2022

Uh oh!

bors commented Jul 4, 2022

Uh oh!

Kixiron commented Jul 4, 2022

Uh oh!

bors commented Jul 4, 2022

Uh oh!

bors commented Jul 4, 2022

Uh oh!

rust-timer commented Jul 5, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants