Don't validate constants in const propagation #109521

tmiasko · 2023-03-23T08:40:27Z

Validation is neither necessary nor desirable.

The constant validation is already omitted at mir-opt-level >= 3, so there there are not changes in MIR test output (the propagation of invalid constants is covered by an existing test in tests/mir-opt/const_prop/invalid_constant.rs).

rustbot · 2023-03-23T08:40:33Z

r? @wesleywiser

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

rustbot · 2023-03-23T08:40:36Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

oli-obk · 2023-03-23T09:08:46Z

@bors try @rust-timer queue

bors · 2023-03-23T09:08:56Z

⌛ Trying commit d2f3831869cf0cef3b9b1c64533e60b69c84e4e4 with merge 4ee8fa53366b8f492b736ddd0d3b28d579df07b4...

tmiasko · 2023-03-23T09:10:50Z

cc @RalfJung

RalfJung · 2023-03-23T10:48:13Z

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -36,16 +36,14 @@ macro_rules! throw_validation_failure {
            msg.push_str(", but expected ");
            write!(&mut msg, $($expected_fmt)*).unwrap();
        )?
-        let path = rustc_middle::ty::print::with_no_trimmed_paths!({


Oh, I hadn't even realized that this was added in the validity checker. Happy to see it gone!

RalfJung · 2023-03-23T10:49:48Z

I'm happy with the interpreter value validity checker change, but can't really comment on the ConstProp diff -- I have no idea why this check was added, and whether it was ever needed for correctness.

bors · 2023-03-23T11:03:51Z

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

bors · 2023-03-23T11:03:51Z

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

rust-timer · 2023-03-23T12:49:17Z

Finished benchmarking commit (4ee8fa53366b8f492b736ddd0d3b28d579df07b4): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

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.6%	[3.6%, 3.6%]	1
Regressions ❌ (secondary)	2.0%	[0.5%, 2.9%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.9%	[-1.5%, -0.5%]	5
All ❌✅ (primary)	3.6%	[3.6%, 3.6%]	1

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.6%	[0.5%, 0.8%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-0.8%, -0.6%]	4
All ❌✅ (primary)	-	-	0

saethlin · 2023-03-23T13:19:59Z

I do not understand the justification for this change. It doesn't make sense to me to justify changing the default behavior of the compiler based on the fact that we test in a special configuration with different behavior.

apiraino · 2023-04-26T15:59:08Z

hello visiting this PR, checking progress.

@tmiasko can you share some thoughts about this comment ? thanks

Validation is neither necessary nor desirable. The validation is already omitted at mir-opt-level >= 3, so there there are not changes in MIR test output (the propagation of invalid constants is covered by an existing test in tests/mir-opt/const_prop/invalid_constant.rs).

The constant validation errors are user facing and should always be emitted to the user - use trimmed path when constructing them.

Since constants are no longer validated before propagation the workaround is obsolete. Remove it.

tmiasko · 2023-04-26T23:17:12Z

Thanks for reminder. The motivation for the change is to remove validation, because as far as I can see it is neither necessary for correctness nor desirable. Maybe Wesley still recalls why it was added in the first place (it started as an assertion)?

Now that CastKind::Transmute work landed, I also removed the workaround it introduced to avoid tripping over the validation.

#109521 (comment) seems to be misreading a sentence about existing test coverage as justification, which indeed makes no sense.

wesleywiser · 2023-05-01T15:42:42Z

I looked through the history and couldn't figure out why I added the validation either. I also checked Zulip to see if there was some discussion or PMs around that time but didn't find anything.

@bors r+

bors · 2023-05-01T15:42:44Z

📌 Commit d1bd1be has been approved by wesleywiser

It is now in the queue for this repository.

bors · 2023-05-02T03:42:41Z

⌛ Testing commit d1bd1be with merge 5133e15...

bors · 2023-05-02T06:36:36Z

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

rust-timer · 2023-05-02T07:53:10Z

Finished benchmarking commit (5133e15): comparison URL.

Overall result: ❌ regressions - 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)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.1%	[-5.7%, -2.4%]	2
Improvements ✅ (secondary)	-3.6%	[-4.3%, -2.9%]	2
All ❌✅ (primary)	-4.1%	[-5.7%, -2.4%]	2

Cycles

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

Binary size

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.2%	[0.1%, 0.2%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.1%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.2%, 0.2%]	5

Bootstrap: 655.55s -> 657.612s (0.31%)

rustbot assigned wesleywiser Mar 23, 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 Mar 23, 2023

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 Mar 23, 2023

RalfJung reviewed Mar 23, 2023

View reviewed changes

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 Mar 23, 2023

tmiasko added 3 commits April 27, 2023 00:32

Use trimmed paths in constantant validation errors

bc41973

The constant validation errors are user facing and should always be emitted to the user - use trimmed path when constructing them.

Remove workaround for CastKind::Transmute from const prop

4adb8fb

Since constants are no longer validated before propagation the workaround is obsolete. Remove it.

tmiasko force-pushed the const-prop-validation branch from d2f3831 to 4adb8fb Compare April 26, 2023 22:54

This comment has been minimized.

Sign in to view

Use EMIT_MIR_FOR_EACH_BIT_WIDTH in transmute mir-opt test

d1bd1be

wesleywiser approved these changes May 1, 2023

View reviewed changes

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 1, 2023

bors mentioned this pull request May 2, 2023

Remove type ascription from parser and diagnostics #109128

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label May 2, 2023

bors merged commit 5133e15 into rust-lang:master May 2, 2023

rustbot added this to the 1.71.0 milestone May 2, 2023

tmiasko deleted the const-prop-validation branch May 2, 2023 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't validate constants in const propagation #109521

Don't validate constants in const propagation #109521

tmiasko commented Mar 23, 2023 •

edited

Loading

rustbot commented Mar 23, 2023

rustbot commented Mar 23, 2023

oli-obk commented Mar 23, 2023

This comment has been minimized.

bors commented Mar 23, 2023

tmiasko commented Mar 23, 2023

RalfJung Mar 23, 2023

RalfJung commented Mar 23, 2023

bors commented Mar 23, 2023

bors commented Mar 23, 2023

This comment has been minimized.

rust-timer commented Mar 23, 2023

saethlin commented Mar 23, 2023

apiraino commented Apr 26, 2023

tmiasko commented Apr 26, 2023

This comment has been minimized.

wesleywiser commented May 1, 2023

bors commented May 1, 2023

bors commented May 2, 2023

bors commented May 2, 2023

rust-timer commented May 2, 2023

Don't validate constants in const propagation #109521

Don't validate constants in const propagation #109521

Conversation

tmiasko commented Mar 23, 2023 • edited Loading

rustbot commented Mar 23, 2023

rustbot commented Mar 23, 2023

oli-obk commented Mar 23, 2023

This comment has been minimized.

bors commented Mar 23, 2023

tmiasko commented Mar 23, 2023

Choose a reason for hiding this comment

RalfJung commented Mar 23, 2023

bors commented Mar 23, 2023

bors commented Mar 23, 2023

This comment has been minimized.

rust-timer commented Mar 23, 2023

Overall result: no relevant changes - no action needed

saethlin commented Mar 23, 2023

apiraino commented Apr 26, 2023

tmiasko commented Apr 26, 2023

This comment has been minimized.

wesleywiser commented May 1, 2023

bors commented May 1, 2023

bors commented May 2, 2023

bors commented May 2, 2023

rust-timer commented May 2, 2023

Overall result: ❌ regressions - no action needed

tmiasko commented Mar 23, 2023 •

edited

Loading