Remove all ConstPropNonsense #119627

oli-obk · 2024-01-05T17:33:14Z

We track all locals and projections on them ourselves within the const propagator and only use the InterpCx to actually do some low level operations or read from constants (via OpTy we get for said constants).

This helps moving the const prop lint out from the normal pipeline and running it just based on borrowck information. This in turn allows us to make progress on #108730 (comment)

there are various follow up cleanups that can be done after this PR (e.g. not matching on Rvalue twice and doing binop checks twice), but lets try landing this one first.

r? @RalfJung

rustbot · 2024-01-05T17:33:24Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

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 · 2024-01-05T17:33:34Z

@bors try @rust-timer queue

… r=<try> Remove all ConstPropNonsense We track all locals and projections on them ourselves within the const propagator and only use the InterpCx to actually do some low level operations or read from constants (via `OpTy` we get for said constants). r? `@RalfJung`

bors · 2024-01-05T17:34:44Z

⌛ Trying commit 878f72f with merge 9d5fcd0...

bors · 2024-01-05T19:00:48Z

☀️ Try build successful - checks-actions
Build commit: 9d5fcd0 (9d5fcd0f21e6b90eb2808d95f23f14feaa72be5c)

rust-timer · 2024-01-05T21:13:12Z

Finished benchmarking commit (9d5fcd0): 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)	-	-	0
Regressions ❌ (secondary)	1.4%	[1.4%, 1.4%]	1
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	10
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.4%, -0.2%]	10

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.9%	[2.9%, 2.9%]	1
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.9%	[2.9%, 2.9%]	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
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.0%	[-6.2%, -5.7%]	5
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 668.199s -> 667.062s (-0.17%)
Artifact size: 311.12 MiB -> 310.97 MiB (-0.05%)

rustbot · 2024-01-08T09:41:59Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler/rustc_mir_transform/src/const_prop.rs

compiler/rustc_mir_transform/src/const_prop_lint.rs

cjgillot · 2024-01-14T17:15:46Z

compiler/rustc_mir_transform/src/const_prop_lint.rs

+                })?;
+                imm.into()
+            }
+        };


We're starting to have quite a few versions of this match on rvalue. There are already one in GVN, one in DataflowConstProp. Do you have any idea how we could deduplicate them?

I have actually copied some logic almost verbatim from GVN and was wondering this myself. It's not clear to me yet if it will be an improvement, but I would like to tackle it so we can at least have an impl that we rejected.

compiler/rustc_mir_transform/src/const_prop_lint.rs

oli-obk · 2024-01-15T14:48:56Z

@bors try @rust-timer queue

bors · 2024-01-15T14:50:07Z

⌛ Trying commit 6405643 with merge e54d71f...

… r=<try> Remove all ConstPropNonsense We track all locals and projections on them ourselves within the const propagator and only use the InterpCx to actually do some low level operations or read from constants (via `OpTy` we get for said constants). This helps moving the const prop lint out from the normal pipeline and running it just based on borrowck information. This in turn allows us to make progress on rust-lang#108730 (comment) there are various follow up cleanups that can be done after this PR (e.g. not matching on Rvalue twice and doing binop checks twice), but lets try landing this one first. r? `@RalfJung`

bors · 2024-01-15T16:16:50Z

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

rust-timer · 2024-01-15T18:12:09Z

Finished benchmarking commit (e54d71f): 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.3%	[-0.4%, -0.2%]	12
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.2%]	9
All ❌✅ (primary)	-0.3%	[-0.4%, -0.2%]	12

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.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	4.5%	[4.3%, 4.7%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[2.5%, 2.5%]	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)	2.1%	[1.3%, 2.6%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 668.966s -> 666.083s (-0.43%)
Artifact size: 308.17 MiB -> 308.05 MiB (-0.04%)

…rojectionElem variant

…e` variant at all

oli-obk · 2024-01-24T12:07:54Z

r? compiler ralf got enough on his plate and most of this is probably not interesting to him

RalfJung · 2024-01-24T16:26:54Z

compiler/rustc_const_eval/src/interpret/projection.rs

+        // However, for now we don't generate such MIR, and this check here *has* found real
+        // bugs (see https://github.com/rust-lang/rust/issues/115145), so we will keep rejecting
+        // it.
+        assert!(!layout.abi.is_uninhabited());


Given that this is causing ICEs and needs extra work-arounds in dataflow-const-prop, I wonder if the more pragmatic choice wouldn't be to just remove the assertion.

RalfJung · 2024-01-24T16:28:47Z

compiler/rustc_mir_transform/src/const_prop.rs

CanConstProp is only used by const_prop_lint, so IMO it makes more sense to move it over there. Then this file can be removed.

I was thinking of making this file a support for the other passes that use an interpreter:

CanConstProp;

the Machine;

...

And eventually common code like arithmetic identities.

That also works, but then it should be called something like const_prop_utils and the module-level comment for the file should clarify its purpose.

cjgillot · 2024-01-24T23:18:09Z

The remaining comments can be addressed in follow-up PRs. I intend to look at the effects on dataflow-const-prop on my side.
@bors r+

bors · 2024-01-24T23:18:12Z

📌 Commit 1c9e621 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2024-01-25T03:16:10Z

⌛ Testing commit 1c9e621 with merge 68411c9...

bors · 2024-01-25T05:15:45Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 68411c9 to master...

rust-timer · 2024-01-25T06:38:24Z

Finished benchmarking commit (68411c9): 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.4%	[-0.6%, -0.2%]	11
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.5%]	3
All ❌✅ (primary)	-0.4%	[-0.6%, -0.2%]	11

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

Binary size

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

Bootstrap: 664.789s -> 662.082s (-0.41%)
Artifact size: 308.40 MiB -> 308.26 MiB (-0.04%)

…n̵sense, r=cjgillot" This reverts commit 68411c9, reversing changes made to 7ffc697.

This reverts commit 68411c9, reversing changes made to 7ffc697. This commit was checked in as PR rust-lang#119627.

rustbot assigned RalfJung Jan 5, 2024

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 Jan 5, 2024

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 Jan 5, 2024

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 Jan 5, 2024

oli-obk force-pushed the const_prop_lint_n̵o̵n̵sense branch from 878f72f to 25b70b3 Compare January 8, 2024 09:11

This comment has been minimized.

Sign in to view

oli-obk mentioned this pull request Jan 14, 2024

Skip lint for assertion evaluated to false #119958

Closed

cjgillot reviewed Jan 14, 2024

View reviewed changes

oli-obk force-pushed the const_prop_lint_n̵o̵n̵sense branch from f6b7ddb to 7ad6b33 Compare January 15, 2024 13:34

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 Jan 15, 2024

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 Jan 15, 2024

oli-obk added 5 commits January 23, 2024 16:35

Implement ConstantIndex handling and use that instead using our own P…

d03eb33

…rojectionElem variant

Remove unnecessary optional layout being passed along

6a01dc9

Inline Index conversion into project method

c5e371d

Switch to using ImmTy instead of OpTy, as we don't use the `MPlac…

271821f

…e` variant at all

No need to check min_length

1c9e621

oli-obk force-pushed the const_prop_lint_n̵o̵n̵sense branch from 5fa508e to 1c9e621 Compare January 23, 2024 16:35

rustbot assigned b-naber and unassigned RalfJung Jan 24, 2024

RalfJung reviewed Jan 24, 2024

View reviewed changes

cjgillot self-assigned this Jan 24, 2024

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, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 25, 2024

bors merged commit 68411c9 into rust-lang:master Jan 25, 2024
12 checks passed

rustbot added this to the 1.77.0 milestone Jan 25, 2024

bors mentioned this pull request Jan 25, 2024

Start emitting overflow lints for const promoted SHL & SHRs #119339

Closed

oli-obk deleted the const_prop_lint_n̵o̵n̵sense branch January 25, 2024 11:34

saethlin mentioned this pull request Jan 25, 2024

ice: !layout.abi.is_uninhabited() #120337

Closed

djkoloski added a commit to djkoloski/rust that referenced this pull request Jan 25, 2024

Revert "Auto merge of rust-lang#119627 - oli-obk:const_prop_lint_n̵o̵…

33c816b

…n̵sense, r=cjgillot" This reverts commit 68411c9, reversing changes made to 7ffc697.

djkoloski mentioned this pull request Jan 25, 2024

Revert #119627 "Remove all ConstPropNonsense" #120347

Closed

djkoloski added a commit to djkoloski/rust that referenced this pull request Jan 25, 2024

Revert rust-lang#119627 "Remove all ConstPropNonsense"

8b2cc26

This reverts commit 68411c9, reversing changes made to 7ffc697. This commit was checked in as PR rust-lang#119627.

matthiaskrgr mentioned this pull request Feb 15, 2024

ICE: assertion failed: value <= 0xFFFF_FF00 #121126

Closed

matthiaskrgr mentioned this pull request Feb 24, 2024

ICE: invalid asymmetric binary op #121534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all ConstPropNonsense #119627

Remove all ConstPropNonsense #119627

oli-obk commented Jan 5, 2024 •

edited

Loading

rustbot commented Jan 5, 2024

oli-obk commented Jan 5, 2024

This comment has been minimized.

bors commented Jan 5, 2024

This comment has been minimized.

bors commented Jan 5, 2024

This comment has been minimized.

rust-timer commented Jan 5, 2024

This comment has been minimized.

rustbot commented Jan 8, 2024

cjgillot Jan 14, 2024

oli-obk Jan 15, 2024

oli-obk commented Jan 15, 2024

This comment has been minimized.

bors commented Jan 15, 2024

bors commented Jan 15, 2024

This comment has been minimized.

rust-timer commented Jan 15, 2024

oli-obk commented Jan 24, 2024

RalfJung Jan 24, 2024

RalfJung Jan 24, 2024

cjgillot Jan 24, 2024

RalfJung Jan 25, 2024

cjgillot commented Jan 24, 2024

bors commented Jan 24, 2024

bors commented Jan 25, 2024

bors commented Jan 25, 2024

rust-timer commented Jan 25, 2024

Remove all ConstPropNonsense #119627

Remove all ConstPropNonsense #119627

Conversation

oli-obk commented Jan 5, 2024 • edited Loading

rustbot commented Jan 5, 2024

oli-obk commented Jan 5, 2024

This comment has been minimized.

bors commented Jan 5, 2024

This comment has been minimized.

bors commented Jan 5, 2024

This comment has been minimized.

rust-timer commented Jan 5, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

This comment has been minimized.

rustbot commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jan 15, 2024

This comment has been minimized.

bors commented Jan 15, 2024

bors commented Jan 15, 2024

This comment has been minimized.

rust-timer commented Jan 15, 2024

Overall result: ✅ improvements - no action needed

oli-obk commented Jan 24, 2024

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

cjgillot commented Jan 24, 2024

bors commented Jan 24, 2024

bors commented Jan 25, 2024

bors commented Jan 25, 2024

rust-timer commented Jan 25, 2024

Overall result: ✅ improvements - no action needed

oli-obk commented Jan 5, 2024 •

edited

Loading