Lower the assume intrinsic to a MIR statement #98332

oli-obk · 2022-06-21T10:09:06Z

This makes #96862 (comment) easier and will generally allow us to cheaply insert assume intrinsic calls in mir building.

r? rust-lang/wg-mir-opt

rust-highfive · 2022-06-21T10:09:10Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive · 2022-06-21T10:09:11Z

r? @wesleywiser

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

oli-obk · 2022-06-21T10:09:23Z

cc @rust-lang/wg-mir-opt

oli-obk · 2022-06-21T10:09:44Z

@bors try @rust-timer queue

rust-timer · 2022-06-21T10:09:46Z

Awaiting bors try build completion.

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

bors · 2022-06-21T10:09:52Z

⌛ Trying commit 15f4f800da144f34374cdaefdf31abeab5465942 with merge ec462c48b5f3225dba4718607e400ee78bdc577d...

compiler/rustc_codegen_cranelift/src/constant.rs

compiler/rustc_middle/src/mir/mod.rs

bors · 2022-06-21T12:02:45Z

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

rust-timer · 2022-06-21T12:02:46Z

Queued ec462c48b5f3225dba4718607e400ee78bdc577d with parent 42dcf70, future comparison URL.

rust-timer · 2022-06-21T16:21:35Z

Finished benchmarking commit (ec462c48b5f3225dba4718607e400ee78bdc577d): comparison url.

Instruction count

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

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

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.1%	3.1%	2
Improvements 🎉 (primary)	-3.5%	-3.5%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-3.5%	-3.5%	1

Cycles

Results

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

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

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.

@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 ↩ ↩² ↩³

compiler/rustc_const_eval/src/interpret/step.rs

wesleywiser

I will give this a more thorough review later this week.

compiler/rustc_middle/src/mir/mod.rs

wesleywiser · 2022-06-22T14:24:27Z

cc @JakobDegen in case you'd like to take a look as well 🙂

JakobDegen · 2022-06-23T19:54:53Z

Thanks for thinking of me Wesley - I actually am on wg-mir-opt though so I had received that ping already 😆

Obviously not a concern that should block this (or indeed any one) PR, but just some thoughts: Adding a new variant to StatementKind is always quite a bit of churn, and as MIR matures and as we add more optimizations that do more complicated things, I expect that it will get more and more error prone to do so.

One way to do this is to do less exhaustive matching in optimizations and try and generalize things to helper functions, but not sure if that's really feasible. A more plausible thing that I could imagine though is us adding a

StatementKind::Intrinsic {
     inputs: Vec<Place>,
    output: Option<Place>,
    intrinsic: IntrinsicKind,
}

#[non_exhaustive]
enum IntrinsicKind { Assume, ... }

The idea here is that this is a fairly general "shape" for a statement to take, and it is likely that large classes of statements that we want to add in the future can be brought into this form. If we then add some requirements about the behavior of these statements (eg may only write to the output place), we can have optimizations provide a fall-through case on IntrinsicKind, and so adding a new one is easy. Optimizations that want to give special treatment to a particular intrinsic are of course still free to do so.

I'm now wondering if maybe this kind of definition makes more sense for Rvalue too... I feel like a lot of use cases only care about the list of input and output places/operands, and not about which operation is used to do the computation.

compiler/rustc_borrowck/src/lib.rs

compiler/rustc_borrowck/src/invalidation.rs

compiler/rustc_mir_transform/src/check_unsafety.rs

src/test/mir-opt/intrinsics.rs

bors · 2022-06-30T07:18:01Z

☔ The latest upstream changes (presumably #98649) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2022-06-30T08:05:47Z

The idea here is that this is a fairly general "shape" for a statement to take, and it is likely that large classes of statements that we want to add in the future can be brought into this form. If we then add some requirements about the behavior of these statements (eg may only write to the output place), we can have optimizations provide a fall-through case on IntrinsicKind, and so adding a new one is easy. Optimizations that want to give special treatment to a particular intrinsic are of course still free to do so.

I'm now wondering if maybe this kind of definition makes more sense for Rvalue too... I feel like a lot of use cases only care about the list of input and output places/operands, and not about which operation is used to do the computation.

Yea, we talked about this when we created CopyNonOverlapping. Though I guess the idea there was more open (basically a Call statement that doesn't unwind like the terminator). Restricting it just to a set of specific functions exhaustively enumerated via an enum, that seems like something we should explore and see if it helps.

rustbot · 2022-06-30T08:16:27Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

…ement

wesleywiser

One small comment. r=me when you're ready

compiler/rustc_borrowck/src/invalidation.rs

oli-obk · 2022-09-06T15:34:12Z

@bors r=wesleywiser

bors · 2022-09-06T15:34:15Z

📌 Commit a0130e6 has been approved by wesleywiser

It is now in the queue for this repository.

bors · 2022-09-06T21:05:24Z

⌛ Testing commit a0130e6 with merge 2a0411df513195dc79ed466ad848f89ee8269373...

bors · 2022-09-06T22:06:49Z

💔 Test failed - checks-actions

rust-log-analyzer · 2022-09-06T22:19:54Z

The job x86_64-msvc-cargo failed! Check out the build log: (web) (plain)

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

Initialized empty Git repository in D:/a/rust/rust/build/ct/iron/.git/
fatal: Could not parse object 'cf056ea5e8052c1feea6141e40ab0306715a2c33'.
remote: Internal Server Error.
remote: 
fatal: unable to access 'https://github.com/iron/iron/': The requested URL returned error: 500
thread 'main' panicked at 'assertion failed: status.success()', src\tools\cargotest\main.rs:148:13
Build completed unsuccessfully in 0:47:28

oli-obk · 2022-09-07T07:03:14Z

@bors retry

bors · 2022-09-07T09:47:26Z

⌛ Testing commit a0130e6 with merge e7c7aa7...

bors · 2022-09-07T12:28:28Z

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing e7c7aa7 to master...

rust-timer · 2022-09-07T16:09:18Z

Finished benchmarking commit (e7c7aa7): comparison URL.

Overall result: ✅ improvements - 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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-0.9%, -0.7%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-0.9%, -0.7%]	6

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.7%	[3.3%, 4.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.7%	[3.3%, 4.1%]	2

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

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

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

rust-highfive assigned wesleywiser Jun 21, 2022

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

oli-obk added the A-mir-opt Area: MIR optimizations label Jun 21, 2022

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

bjorn3 reviewed Jun 21, 2022

View reviewed changes

compiler/rustc_codegen_cranelift/src/constant.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Jun 21, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Jun 21, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

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

RalfJung reviewed Jun 21, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/step.rs Outdated Show resolved Hide resolved

wesleywiser reviewed Jun 22, 2022

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

RalfJung mentioned this pull request Jun 25, 2022

Change enum->int casts to not go through MIR casts. #96862

Merged

wesleywiser reviewed Jun 27, 2022

View reviewed changes

compiler/rustc_borrowck/src/lib.rs Outdated Show resolved Hide resolved

compiler/rustc_borrowck/src/invalidation.rs Outdated Show resolved Hide resolved

tmiasko reviewed Jun 29, 2022

View reviewed changes

compiler/rustc_mir_transform/src/check_unsafety.rs Outdated Show resolved Hide resolved

src/test/mir-opt/intrinsics.rs Outdated Show resolved Hide resolved

oli-obk force-pushed the assume branch from 81e1f11 to e6d58ce Compare June 30, 2022 08:16

oli-obk added 3 commits September 6, 2022 14:18

Generalize the Assume intrinsic statement to a general Intrinsic stat…

b741351

…ement

Move CTFE handling of nondiverging intrinsics to intrinsics.rs

104f97e

Update docs

7348284

oli-obk force-pushed the assume branch from 331ba00 to 7348284 Compare September 6, 2022 14:18

This comment has been minimized.

Sign in to view

Test bump

cd928b0

wesleywiser approved these changes Sep 6, 2022

View reviewed changes

compiler/rustc_borrowck/src/invalidation.rs Show resolved Hide resolved

Check all operands, they may contain indirections in their place

a0130e6

oli-obk force-pushed the assume branch from cfdbddc to a0130e6 Compare September 6, 2022 15:33

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 Sep 6, 2022

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 Sep 6, 2022

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 Sep 7, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 7, 2022

bors merged commit e7c7aa7 into rust-lang:master Sep 7, 2022

rustbot added this to the 1.65.0 milestone Sep 7, 2022

oli-obk deleted the assume branch September 7, 2022 18:28

nikic mentioned this pull request Sep 26, 2022

Array indexed by enum cast to usize no longer has bounds check eliminated #102303

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lower the assume intrinsic to a MIR statement #98332

Lower the assume intrinsic to a MIR statement #98332

oli-obk commented Jun 21, 2022

rust-highfive commented Jun 21, 2022

rust-highfive commented Jun 21, 2022

oli-obk commented Jun 21, 2022

oli-obk commented Jun 21, 2022

rust-timer commented Jun 21, 2022

bors commented Jun 21, 2022

bors commented Jun 21, 2022

rust-timer commented Jun 21, 2022

rust-timer commented Jun 21, 2022

wesleywiser left a comment

wesleywiser commented Jun 22, 2022

JakobDegen commented Jun 23, 2022

bors commented Jun 30, 2022

oli-obk commented Jun 30, 2022

rustbot commented Jun 30, 2022

This comment has been minimized.

wesleywiser left a comment

oli-obk commented Sep 6, 2022

bors commented Sep 6, 2022

bors commented Sep 6, 2022

bors commented Sep 6, 2022

rust-log-analyzer commented Sep 6, 2022

oli-obk commented Sep 7, 2022

bors commented Sep 7, 2022

bors commented Sep 7, 2022

rust-timer commented Sep 7, 2022

Lower the assume intrinsic to a MIR statement #98332

Lower the assume intrinsic to a MIR statement #98332

Conversation

oli-obk commented Jun 21, 2022

rust-highfive commented Jun 21, 2022

rust-highfive commented Jun 21, 2022

oli-obk commented Jun 21, 2022

oli-obk commented Jun 21, 2022

rust-timer commented Jun 21, 2022

bors commented Jun 21, 2022

bors commented Jun 21, 2022

rust-timer commented Jun 21, 2022

rust-timer commented Jun 21, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

wesleywiser left a comment

Choose a reason for hiding this comment

wesleywiser commented Jun 22, 2022

JakobDegen commented Jun 23, 2022

bors commented Jun 30, 2022

oli-obk commented Jun 30, 2022

rustbot commented Jun 30, 2022

This comment has been minimized.

wesleywiser left a comment

Choose a reason for hiding this comment

oli-obk commented Sep 6, 2022

bors commented Sep 6, 2022

bors commented Sep 6, 2022

bors commented Sep 6, 2022

rust-log-analyzer commented Sep 6, 2022

oli-obk commented Sep 7, 2022

bors commented Sep 7, 2022

bors commented Sep 7, 2022

rust-timer commented Sep 7, 2022

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes