InstCombine away intrinsic validity assertions #105582

saethlin · 2022-12-12T02:08:38Z

This optimization (currently) fires 246 times on the standard library. It seems to fire hardly at all on the big crates in the benchmark suite. Interesting.

rustbot · 2022-12-12T02:08:43Z

r? @petrochenkov

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

rustbot · 2022-12-12T02:09:11Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

saethlin · 2022-12-12T02:11:03Z

@bors try @rust-timer queue

bors · 2022-12-12T02:11:13Z

⌛ Trying commit 2d86c3c5de3b4cf8c1a7f0d51f5466a1b2120586 with merge 7893106e07baf9fba3abcd2391134f6cf939f6f2...

bors · 2022-12-12T04:57:03Z

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

rust-timer · 2022-12-12T06:14:44Z

Finished benchmarking commit (7893106e07baf9fba3abcd2391134f6cf939f6f2): 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.4%	[1.4%, 1.4%]	1
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
All ❌✅ (primary)	-0.6%	[-0.6%, -0.6%]	1

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)	2.1%	[1.0%, 3.3%]	2
Regressions ❌ (secondary)	2.2%	[1.7%, 2.9%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.9%	[-0.9%, -0.9%]	1
All ❌✅ (primary)	2.1%	[1.0%, 3.3%]	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)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

oli-obk · 2022-12-12T08:02:52Z

r? @oli-obk

compiler/rustc_mir_transform/src/instcombine.rs

saethlin · 2023-01-09T14:55:12Z

Waiting on DestProp improvement
@rustbot label +S-blocked

src/test/mir-opt/intrinsic_asserts.rs

src/test/mir-opt/intrinsic_asserts.main.InstCombine.diff

saethlin · 2023-01-16T17:48:50Z

Not sure why I said this was blocked on DestProp improvement. I didn't want this merged originally because it made #106141 easier to hit. But that issue is fixed now thanks to @cjgillot so I think this is good to go.

@rustbot label -S-blocked

oli-obk · 2023-01-16T18:49:21Z

r? @cjgillot feel free to bounce back if preferred

bors · 2023-01-21T10:43:25Z

⌛ Testing commit 662199f with merge ed9d19a24db785dd6b0b49eead9328842892e10e...

bors · 2023-01-21T11:02:23Z

💔 Test failed - checks-actions

cjgillot · 2023-01-21T16:23:00Z

Failure is legit. The permits_{uninit,zero}_init queries should take a ParamEnv instead of inventing their own.

rustbot · 2023-01-22T22:11:29Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

compiler/rustc_middle/src/query/mod.rs

cjgillot · 2023-01-24T20:21:04Z

@bors r+

bors · 2023-01-24T20:21:06Z

📌 Commit 5bfad5c has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-01-26T03:10:55Z

⌛ Testing commit 5bfad5c with merge 885bf62...

bors · 2023-01-26T06:23:07Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 885bf62 to master...

rust-timer · 2023-01-26T07:43:04Z

Finished benchmarking commit (885bf62): 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.2%	[-0.3%, -0.2%]	6
Improvements ✅ (secondary)	-0.6%	[-1.3%, -0.3%]	4
All ❌✅ (primary)	-0.2%	[-0.3%, -0.2%]	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)	2.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.8%	[2.8%, 2.8%]	1

Cycles

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

RalfJung · 2023-01-30T19:49:45Z

compiler/rustc_mir_transform/src/instcombine.rs

+            *target = None;
+        } else {
+            // If we know the assert does not panic, turn the call into a Goto
+            terminator.kind = TerminatorKind::Goto { target: *target_block };


I don't think I entirely understand the point of this. The intrinsic will anyway compile to a goto in this case. It's not like this ever becomes a runtime check. So what is the win here?

After SimplifyCfg the goto will be gone, shrinking the mir slightly. Also having to evaluate the intrinsic every time you monomorphize is slightly more expensive than evaluating it once at mir optimization time.

…d, r=cjgillot InstCombine away intrinsic validity assertions This optimization (currently) fires 246 times on the standard library. It seems to fire hardly at all on the big crates in the benchmark suite. Interesting.

(GeneratorWitnessMIR) Compute generator saved locals on MIR rust-lang/rust#101692 (ParamEnv) InstCombine away intrinsic validity assertions rust-lang/rust#105582 (Primitive::Pointer) abi: add AddressSpace field to Primitive::Pointer rust-lang/rust#107248

rustbot assigned petrochenkov Dec 12, 2022

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 Dec 12, 2022

saethlin marked this pull request as ready for review December 12, 2022 02:09

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 Dec 12, 2022

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 Dec 12, 2022

rustbot assigned oli-obk and unassigned petrochenkov Dec 12, 2022

oli-obk reviewed Dec 12, 2022

View reviewed changes

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

saethlin force-pushed the instcombine-assert-inhabited branch from dcb9c47 to a22a62a Compare December 12, 2022 20:33

rustbot added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jan 9, 2023

cjgillot reviewed Jan 15, 2023

View reviewed changes

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

src/test/mir-opt/intrinsic_asserts.main.InstCombine.diff Outdated Show resolved Hide resolved

InstCombine away intrinsic validity assertions

662199f

saethlin force-pushed the instcombine-assert-inhabited branch from a22a62a to 662199f Compare January 15, 2023 21:52

rustbot removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jan 16, 2023

rustbot assigned cjgillot and unassigned oli-obk Jan 16, 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 Jan 21, 2023

This comment has been minimized.

Sign in to view

cjgillot reviewed Jan 23, 2023

View reviewed changes

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

Thread a ParamEnv down to might_permit_raw_init

5bfad5c

saethlin force-pushed the instcombine-assert-inhabited branch from 80bd34a to 5bfad5c Compare January 24, 2023 00:25

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 Jan 24, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 26, 2023

bors merged commit 885bf62 into rust-lang:master Jan 26, 2023

rustbot added this to the 1.69.0 milestone Jan 26, 2023

bors mentioned this pull request Jan 26, 2023

[unused_braces] Lint multiline blocks as long as not in arms #102432

Closed

RalfJung reviewed Jan 30, 2023

View reviewed changes

tedinski mentioned this pull request Feb 3, 2023

Update Rust toolchain to nightly-2023-02-03 model-checking/kani#2184

Closed

4 tasks

saethlin mentioned this pull request Feb 13, 2023

ICE: need to be able to compute layouts: Unknown(T) running miri with -Zunpretty-mir #107999

Closed

qinheping mentioned this pull request Mar 9, 2023

Upgrade Rust toolchain to nightly-2023-02-03 model-checking/kani#2291

Merged

4 tasks

saethlin deleted the instcombine-assert-inhabited branch March 15, 2023 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InstCombine away intrinsic validity assertions #105582

InstCombine away intrinsic validity assertions #105582

saethlin commented Dec 12, 2022 •

edited

Loading

rustbot commented Dec 12, 2022

rustbot commented Dec 12, 2022

saethlin commented Dec 12, 2022

This comment has been minimized.

bors commented Dec 12, 2022

bors commented Dec 12, 2022

This comment has been minimized.

rust-timer commented Dec 12, 2022

oli-obk commented Dec 12, 2022

saethlin commented Jan 9, 2023

saethlin commented Jan 16, 2023

oli-obk commented Jan 16, 2023

bors commented Jan 21, 2023

bors commented Jan 21, 2023

This comment has been minimized.

cjgillot commented Jan 21, 2023

rustbot commented Jan 22, 2023

cjgillot commented Jan 24, 2023

bors commented Jan 24, 2023

bors commented Jan 26, 2023

bors commented Jan 26, 2023

rust-timer commented Jan 26, 2023

RalfJung Jan 30, 2023

bjorn3 Jan 30, 2023

InstCombine away intrinsic validity assertions #105582

InstCombine away intrinsic validity assertions #105582

Conversation

saethlin commented Dec 12, 2022 • edited Loading

rustbot commented Dec 12, 2022

rustbot commented Dec 12, 2022

saethlin commented Dec 12, 2022

This comment has been minimized.

bors commented Dec 12, 2022

bors commented Dec 12, 2022

This comment has been minimized.

rust-timer commented Dec 12, 2022

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

oli-obk commented Dec 12, 2022

saethlin commented Jan 9, 2023

saethlin commented Jan 16, 2023

oli-obk commented Jan 16, 2023

bors commented Jan 21, 2023

bors commented Jan 21, 2023

This comment has been minimized.

cjgillot commented Jan 21, 2023

rustbot commented Jan 22, 2023

cjgillot commented Jan 24, 2023

bors commented Jan 24, 2023

bors commented Jan 26, 2023

bors commented Jan 26, 2023

rust-timer commented Jan 26, 2023

Overall result: ✅ improvements - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin commented Dec 12, 2022 •

edited

Loading