Only rewrite valtree-constants to patterns and keep other constants opaque #111913

oli-obk · 2023-05-24T15:38:03Z

Now that we can reliably fall back to comparing constants with PartialEq::eq to the match scrutinee, we can

eagerly try to convert constants to valtrees
then deeply convert the valtree to a pattern
if the to-valtree conversion failed, create an "opaque constant" pattern.

This PR specifically avoids any behavioral changes or major cleanups. What we can now do as follow ups is

move the two remaining call sites to destructure_mir_constant off that query
make valtree to pattern conversion infallible
- this needs to be done after careful analysis of the effects. There may be user visible changes from that.

rustbot · 2023-05-24T15:38:12Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2023-05-24T15:38:15Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

oli-obk · 2023-05-24T15:39:13Z

@bors try @rust-timer queue

bors · 2023-05-24T15:39:22Z

⌛ Trying commit ab7fadeec91e3e063d7f3588ce5c63add2c41c6d with merge 8ddab403da8861e4c837620984384a5adc5078d5...

bors · 2023-05-24T17:49:43Z

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

rust-timer · 2023-05-24T19:05:08Z

Finished benchmarking commit (8ddab403da8861e4c837620984384a5adc5078d5): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

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-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)	9.5%	[5.7%, 13.9%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	8.1%	[-0.3%, 13.9%]	7

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.4%	[3.4%, 3.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.4%	[3.4%, 3.4%]	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)	5.2%	[3.2%, 7.8%]	6
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.7%	[-3.7%, -3.7%]	1
All ❌✅ (primary)	5.2%	[3.2%, 7.8%]	6

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

Bootstrap: 647.458s -> 647.023s (-0.07%)

compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs

oli-obk · 2023-05-25T13:25:38Z

@bors try @rust-timer queue

bors · 2023-05-25T13:25:48Z

⌛ Trying commit 745e7c3d2653d8f51fe3ecb87c640f0df8c70271 with merge 575c9022686fc5f7b869e4eac870bb5267d30553...

bors · 2023-05-25T15:47:02Z

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

rust-timer · 2023-05-25T17:31:19Z

Finished benchmarking commit (575c9022686fc5f7b869e4eac870bb5267d30553): comparison URL.

Overall result: ✅ 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)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.3%]	5
All ❌✅ (primary)	-0.7%	[-0.7%, -0.7%]	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)	-	-	0
Regressions ❌ (secondary)	2.4%	[1.8%, 3.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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

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

Bootstrap: 646.591s -> 645.916s (-0.10%)

lcnr · 2023-05-26T08:03:07Z

r? @lcnr

oli-obk · 2023-05-31T14:28:11Z

@bors r=lcnr

bors · 2023-05-31T14:28:13Z

📌 Commit 3c02cfc has been approved by lcnr

It is now in the queue for this repository.

bors · 2023-05-31T16:36:54Z

⌛ Testing commit 3c02cfc with merge 871b595...

bors · 2023-05-31T19:25:56Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 871b595 to master...

rust-timer · 2023-05-31T20:41:50Z

Finished benchmarking commit (871b595): comparison URL.

Overall result: ❌✅ regressions and 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.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-0.3%, 0.7%]	2

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)	-0.1%	[-0.1%, -0.1%]	1
Improvements ✅ (secondary)	-3.0%	[-3.5%, -2.5%]	2
All ❌✅ (primary)	-0.1%	[-0.1%, -0.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: 642.658s -> 643.718s (0.16%)

RalfJung · 2023-06-16T14:24:38Z

compiler/rustc_const_eval/src/const_eval/mod.rs

@@ -131,38 +129,3 @@ pub(crate) fn try_destructure_mir_constant<'tcx>(

    Ok(mir::DestructuredConstant { variant, fields })
 }
-
-#[instrument(skip(tcx), level = "debug")]
-pub(crate) fn deref_mir_constant<'tcx>(


So happy to see this go. :)

Now, when will destructure_mir_constant disappear? ;)
It seems to be used only for pretty-printing, and for some obscure SIMD-related thing.

yea, the SIMD thing I wanna solve first, and then just move destructure_mir_constant into pretty printing and kill the query

Nice. :)

What's the solution for the SIMD thing? Just go through valtree?

I only had a superficial look so far, and I didn't come up with a good solution then and decided to look at it later :D

use `PatKind::wild` when an ADT const value has violation Fixes rust-lang#115599 Since the [to_pat](https://github.com/rust-lang/rust/pull/111913/files#diff-6d8d99538aca600d633270051580c7a9e40b35824ea2863d9dda2c85a733b5d9R126-R155) behavior has been changed in the rust-lang#111913 update, the kind of `inlined_const_ast_pat` has transformed from `PatKind::Leaf { pattern: Pat { kind: Wild, ..} } ` to `PatKind::Constant`. This caused a scenario where there are no matched candidates, leading to a testing of the candidates. This process ultimately attempts to test the string const, triggering the `bug!` invocation finally. r? `@oli-obk`

use `PatKind::Error` when an ADT const value has violation Fixes rust-lang#115599 Since the [to_pat](https://github.com/rust-lang/rust/pull/111913/files#diff-6d8d99538aca600d633270051580c7a9e40b35824ea2863d9dda2c85a733b5d9R126-R155) behavior has been changed in the rust-lang#111913 update, the kind of `inlined_const_ast_pat` has transformed from `PatKind::Leaf { pattern: Pat { kind: Wild, ..} } ` to `PatKind::Constant`. This caused a scenario where there are no matched candidates, leading to a testing of the candidates. This process ultimately attempts to test the string const, triggering the `bug!` invocation finally. r? `@oli-obk`

use `PatKind::Error` when an ADT const value has violation Fixes rust-lang#115599 Since the [to_pat](https://github.com/rust-lang/rust/pull/111913/files#diff-6d8d99538aca600d633270051580c7a9e40b35824ea2863d9dda2c85a733b5d9R126-R155) behavior has been changed in the rust-lang#111913 update, the kind of `inlined_const_ast_pat` has transformed from `PatKind::Leaf { pattern: Pat { kind: Wild, ..} } ` to `PatKind::Constant`. This caused a scenario where there are no matched candidates, leading to a testing of the candidates. This process ultimately attempts to test the string const, triggering the `bug!` invocation finally. r? ``@oli-obk``

Rollup merge of rust-lang#116522 - bvanjoi:fix-115599, r=oli-obk use `PatKind::Error` when an ADT const value has violation Fixes rust-lang#115599 Since the [to_pat](https://github.com/rust-lang/rust/pull/111913/files#diff-6d8d99538aca600d633270051580c7a9e40b35824ea2863d9dda2c85a733b5d9R126-R155) behavior has been changed in the rust-lang#111913 update, the kind of `inlined_const_ast_pat` has transformed from `PatKind::Leaf { pattern: Pat { kind: Wild, ..} } ` to `PatKind::Constant`. This caused a scenario where there are no matched candidates, leading to a testing of the candidates. This process ultimately attempts to test the string const, triggering the `bug!` invocation finally. r? ``@oli-obk``

rustbot assigned davidtwco May 24, 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 May 24, 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 May 24, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 24, 2023

Nadrieril reviewed May 24, 2023

View reviewed changes

compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs Outdated Show resolved Hide resolved

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

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels May 25, 2023

rustbot assigned lcnr May 26, 2023

oli-obk added 7 commits May 31, 2023 14:07

Remove lit_to_mir_constant query

4ca8707

Remove some dead code

1722aa7

Simplify an if let Some to a ?

bd4197c

bug! message nit

9cf7810

Explain and simplify valtree -> mir-const fallback

c4d5dde

Inline from_inline_const into its sole call site

81b07ed

Explain the reason for why a test exists

3c02cfc

oli-obk force-pushed the valtrees2 branch from aaa7e55 to 3c02cfc Compare May 31, 2023 14:07

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

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

bors merged commit 871b595 into rust-lang:master May 31, 2023

rustbot added this to the 1.72.0 milestone May 31, 2023

oli-obk deleted the valtrees2 branch May 31, 2023 22:04

oli-obk mentioned this pull request Jun 4, 2023

incremental ICE: slice-pattern-const.rs #83085

Closed

matthiaskrgr mentioned this pull request Jun 11, 2023

ICE assertion failed: matches!(self.def_kind(ct.def), DefKind :: AnonConst) if fn has generic param #112438

Closed

RalfJung reviewed Jun 16, 2023

View reviewed changes

mj10021 mentioned this pull request Jun 25, 2023

ICE with range pattern inside struct initializer at compile time #113012

Closed

matthiaskrgr mentioned this pull request Sep 6, 2023

ICE error when matching uninitialized constant with constant pattern in Rust #115599

Closed

bvanjoi mentioned this pull request Oct 7, 2023

use PatKind::Error when an ADT const value has violation #116522

Merged

matthiaskrgr mentioned this pull request Feb 3, 2024

ice: named field access on non-ADT #120615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only rewrite valtree-constants to patterns and keep other constants opaque #111913

Only rewrite valtree-constants to patterns and keep other constants opaque #111913

oli-obk commented May 24, 2023 •

edited

Loading

rustbot commented May 24, 2023

rustbot commented May 24, 2023

oli-obk commented May 24, 2023

This comment has been minimized.

bors commented May 24, 2023

This comment has been minimized.

bors commented May 24, 2023

This comment has been minimized.

rust-timer commented May 24, 2023

oli-obk commented May 25, 2023

This comment has been minimized.

bors commented May 25, 2023

This comment has been minimized.

bors commented May 25, 2023

This comment has been minimized.

rust-timer commented May 25, 2023

lcnr commented May 26, 2023

oli-obk commented May 31, 2023

bors commented May 31, 2023

bors commented May 31, 2023

bors commented May 31, 2023

rust-timer commented May 31, 2023

RalfJung Jun 16, 2023

oli-obk Jun 16, 2023

RalfJung Jun 16, 2023

oli-obk Jun 16, 2023

Only rewrite valtree-constants to patterns and keep other constants opaque #111913

Only rewrite valtree-constants to patterns and keep other constants opaque #111913

Conversation

oli-obk commented May 24, 2023 • edited Loading

rustbot commented May 24, 2023

rustbot commented May 24, 2023

oli-obk commented May 24, 2023

This comment has been minimized.

bors commented May 24, 2023

This comment has been minimized.

bors commented May 24, 2023

This comment has been minimized.

rust-timer commented May 24, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

oli-obk commented May 25, 2023

This comment has been minimized.

bors commented May 25, 2023

This comment has been minimized.

bors commented May 25, 2023

This comment has been minimized.

rust-timer commented May 25, 2023

Overall result: ✅ improvements - no action needed

lcnr commented May 26, 2023

oli-obk commented May 31, 2023

bors commented May 31, 2023

bors commented May 31, 2023

bors commented May 31, 2023

rust-timer commented May 31, 2023

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

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

oli-obk commented May 24, 2023 •

edited

Loading