Run const_prop_lint in check builds, too #120687

oli-obk · 2024-02-05T22:40:38Z

implements #108730 (comment)

addresses a large part of #49292

Turns the const_prop_lint pass into a query that is run alongside borrowck.

rustbot · 2024-02-05T22:40:47Z

r? @fmease

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

rustbot · 2024-02-05T22:40:49Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

oli-obk · 2024-02-05T22:40:50Z

@bors try @rust-timer queue

bors · 2024-02-05T22:42:00Z

⌛ Trying commit 74cde5f with merge 63a199c...

Run const_prop_lint in check builds, too implements rust-lang#108730 (comment) Turns the const_prop_lint pass into a query that is run alongside borrowck.

oli-obk · 2024-02-05T22:42:34Z

compiler/rustc_abi/src/layout.rs

-        _ => assert!(
-            start == Bound::Unbounded && end == Bound::Unbounded,
-            "nonscalar layout for layout_scalar_valid_range type: {st:#?}",
-        ),
+        _ => {
+            if start != Bound::Unbounded || end != Bound::Unbounded {
+                return None;
+            }
+        }


Can be reverted, this is hit when rustc_scalar_layout_start is used on structs with a non-scalar field. We can just stop testing that. It's all unstable anyway.

oli-obk · 2024-02-05T22:43:55Z

compiler/rustc_mir_transform/src/lib.rs

+    // If there are impossible bounds on the body being const prop linted,
+    // the const eval logic used in const prop may ICE unexpectedly.
+    let predicates = tcx
+        .predicates_of(body.source.def_id())
+        .predicates
+        .iter()
+        .filter_map(|(p, _)| if p.is_global() { Some(*p) } else { None });
+    if !traits::impossible_predicates(tcx, traits::elaborate(tcx, predicates).collect())


This duplication is somewhat unfortunate. We could make const_prop_lint return whether there were impossible predicates and then bail out.

Or we perform this in borrowck and store the information in borrowck results.

If perf doesn't find this problematic, we could just deduplicate by moving the logic into a function

It may be interesting to look at error taint before trying to check for impossible predicates, as that's faster.
IIUC, this impossible predicates check is more of a heuristic to avoid diagnostic spam, as it does not try to prove satisfiability, does it?

It may be interesting to look at error taint before trying to check for impossible predicates, as that's faster.

there are no errors usually. With trivial_bounds you may have a broken function that will only error if called, never out of another reason.

cjgillot · 2024-02-06T00:15:47Z

compiler/rustc_mir_transform/src/lib.rs

@@ -394,6 +395,27 @@ fn inner_mir_for_ctfe(tcx: TyCtxt<'_>, def: LocalDefId) -> Body<'_> {
    body
 }

+fn const_prop_lint(tcx: TyCtxt<'_>, def: LocalDefId) {


Can we use this as an opportunity to use a better name? lint_arithmetic_overflow ?

oli-obk · 2024-02-06T07:19:03Z

@rust-timer build 63a199c

rust-timer · 2024-02-06T09:43:04Z

Finished benchmarking commit (63a199c): comparison URL.

Overall result: ❌ regressions - 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)	1.5%	[0.2%, 9.4%]	109
Regressions ❌ (secondary)	2.1%	[0.3%, 18.0%]	43
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.5%	[-0.2%, 9.4%]	111

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.1%	[0.9%, 15.9%]	25
Regressions ❌ (secondary)	5.3%	[1.0%, 12.5%]	10
Improvements ✅ (primary)	-1.9%	[-2.5%, -1.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[-2.5%, 15.9%]	28

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)	3.0%	[0.9%, 10.4%]	62
Regressions ❌ (secondary)	4.2%	[1.4%, 20.5%]	23
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[0.9%, 10.4%]	62

Binary size

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

Bootstrap: 660.166s -> 662.022s (0.28%)
Artifact size: 308.14 MiB -> 308.18 MiB (0.01%)

oli-obk · 2024-02-06T09:47:36Z

Lower mean and max regression in primary benchmarks than #108730 (comment) But all that extra work is going to cost us some perf in check builds, no matter what.

oli-obk · 2024-02-06T09:49:38Z

most of the cost is in eval_to_allocation_raw. So one thing that we could do is avoid evaluating constants from other crates during const prop in check builds. We should already be evaluating all local constants anyway. This will reduce const prop coverage somewhat in check builds, but only if the errors rely on constants from other crates.

cjgillot · 2024-02-07T00:07:40Z

Any idea why we call eval_to_allocation_raw more? The PR does not strike me as adding much work, but moves the existing.
Are we evaluating some constant multiple times? For instance, once in const_prop_lint, once in mir_for_ctfe and once in optimized_mir?

oli-obk · 2024-02-07T10:27:46Z

The difference is that we didn't (reliably) run const prop lints in check builds at all before this PR. Now we do. So a regression due to extra work is expected.

bors · 2024-02-08T07:56:03Z

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

rustbot · 2024-02-08T08:58:14Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

oli-obk · 2024-02-08T10:02:49Z

@bors try @rust-timer queue

bors · 2024-02-08T10:04:02Z

⌛ Trying commit f271893 with merge 460bf3d...

Run const_prop_lint in check builds, too implements rust-lang#108730 (comment) Turns the const_prop_lint pass into a query that is run alongside borrowck.

bors · 2024-02-08T11:33:52Z

☀️ Try build successful - checks-actions
Build commit: 460bf3d (460bf3de149d238b1df24c63883e2612eae5c529)

rust-timer · 2024-02-08T14:43:07Z

Finished benchmarking commit (460bf3d): comparison URL.

Overall result: ❌ regressions - 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)	1.5%	[0.2%, 9.3%]	106
Regressions ❌ (secondary)	2.1%	[0.2%, 17.9%]	38
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.2%]	7
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	1.4%	[-0.3%, 9.3%]	113

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%	[1.1%, 7.7%]	12
Regressions ❌ (secondary)	6.9%	[2.4%, 11.8%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-5.5%, -2.2%]	4
All ❌✅ (primary)	2.8%	[1.1%, 7.7%]	12

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)	3.0%	[1.4%, 10.1%]	56
Regressions ❌ (secondary)	5.1%	[2.0%, 19.7%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-4.8%, -3.3%]	4
All ❌✅ (primary)	3.0%	[1.4%, 10.1%]	56

Binary size

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

Bootstrap: 662.662s -> 663.487s (0.12%)
Artifact size: 308.26 MiB -> 308.23 MiB (-0.01%)

oli-obk · 2024-02-08T16:56:56Z

Looks like all the regressions are legit. Check builds now actually running the lints or non-check-builds in incremental doing some incremental cache loading that they didn't do before

fmease · 2024-03-01T10:01:21Z

I haven't reviewed this yet since I'm assuming this is effectively S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and I don't have enough knowledge of the query system to know how to combat the perf regressions. However if this is actually S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. , I can gladly try to review it to the best of my ability.

oli-obk · 2024-03-01T10:13:00Z

Oh I basically think the regression cannot be avoided 😆 Either we emit those lints in check builds, and thus take a perf hit on check builds, or we decide to take the check vs build difference in lints.

cc @nnethercote because you expressed concerns about perf regressions due to such changes.

rustbot assigned fmease Feb 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 Feb 5, 2024