move required_consts check to general post-mono-check function #115748

RalfJung · 2023-09-11T08:00:44Z

This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same eval functions as everyone else to get the evaluated MIR constants.

Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) #115709: ensuring that all locals are dynamically sized.

I didn't expect this to change diagnostics, but it's just cycle errors that change.

r? @oli-obk

rustbot · 2023-09-11T08:00:55Z

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

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 the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

RalfJung · 2023-09-11T08:02:32Z

compiler/rustc_middle/src/mir/mod.rs

+                    | ty::ConstKind::Expr(..) => return Err(ErrorHandled::TooGeneric(const_.span)),
+                },
+                ConstantKind::Unevaluated(uv, _) => uv,
+                ConstantKind::Val(..) => return Ok(()),


We have this kind of match in a bunch of places (here, interpret, codegen_ssa, codegen_cranelift). There doesn't happen to be some eval function we can call instead? There are eval methods on constants but they return a constant again, i.e. the return type doesn't reflect that anything was evaluated -- not quite what I was expecting from an eval function.

It's not clear what we can return from such an eval method, we'd need a new enum that can represent Valtree | ConstValue. But yea, would be nice if we could share more code in situations like this

Why do we need a new enum? For ty::Const it's a Valtree, for mir::Const it's a ConstValue.

mir::Const would then need to convert Valtree to ConstValue in case it starts out with an evaluated ty::Const.

Yeah, that's what I would expect to happen, indeed. Miri/const-eval already do that.

This seems to actually have worked. :)

bjorn3 · 2023-09-11T08:11:18Z

compiler/rustc_codegen_ssa/src/mir/mod.rs

+            cx.tcx().sess.diagnostic().emit_bug(errors::PolymorphicConstantTooGeneric { span });
+        }
+        Err(ErrorHandled::Reported(_, span)) => {
+            cx.tcx().sess.emit_err(errors::ErroneousConstant { span });


Maybe handle TooGeneric entirely inside post_mono_checks, and for Reported emit the error inside it and return Err(()) to allow the backend to skip codegen for the function?

One of the goals was to share this code with const-eval, and in const-eval TooGeneric can unfortunately occur since const generics will (try to) evaluate generic MIR. At least I got tons of ICEs when I tried to handle TooGeneric in post_mono_checks. (There's a doc comment at post_mono_checks explaining this.)

rustbot · 2023-09-11T08:45:35Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

tests/ui/consts/issue-70942-trait-vs-impl-mismatch.stderr

compiler/rustc_codegen_ssa/src/mir/constant.rs

rustbot · 2023-09-11T10:50:33Z

The Miri subtree was changed

cc @rust-lang/miri

compiler/rustc_middle/src/ty/consts.rs

RalfJung · 2023-09-11T11:22:31Z

@bors try @rust-timer queue

bors · 2023-09-11T11:22:42Z

⌛ Trying commit e0b833e with merge 0ae715c...

move required_consts check to general post-mono-check function I was hoping this would simplify the code. That didn't entirely happen, but I still think it could be useful to have a single place for the "required const" check -- and this does simplify some code in codegen, where the const-eval functions no longer have to return a `Result`. Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang#115709: ensuring that all locals are dynamically sized. I didn't expect this to change diagnostics, but the changes seem either harmless (for the cycle errors) or improvements (where more spans are shown when something goes wrong in a constant). r? `@oli-obk`

RalfJung · 2023-09-14T20:35:25Z

I was worried about regressions and I'm getting improvements. I'll take it. :) For the stress test that even more than evens out the regressions from my other changes the last weeks. ;)

@rustbot ready

make the eval() functions on our const types return the resulting value This is a part of rust-lang/rust#115748 that's hopefully perf-neutral, and that does not depend on rust-lang/rust#115764.

Do not clone the Body for ConstProp ~Based on rust-lang/rust#115748 for the `POST_MONO_CHECKS` flag.~

consistently pass ty::Const through valtrees Some drive-by things extracted from rust-lang/rust#115748.

oli-obk · 2023-09-18T09:48:17Z

@bors r+

bors · 2023-09-18T09:48:19Z

📌 Commit 9ac8b36 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-09-18T13:49:55Z

⌛ Testing commit 9ac8b36 with merge 4937664...

move required_consts check to general post-mono-check function This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same `eval` functions as everyone else to get the evaluated MIR constants. Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang#115709: ensuring that all locals are dynamically sized. I didn't expect this to change diagnostics, but it's just cycle errors that change. r? `@oli-obk`

rust-log-analyzer · 2023-09-18T14:25:58Z

The job dist-aarch64-linux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2023-09-18T14:26:34Z

💔 Test failed - checks-actions

oli-obk · 2023-09-18T14:54:17Z

@bors retry failed to download from [https://crates.io/api/v1/crates/minifier/0.2.2/download](https://crates.io/api/v1/crates/minifier/0.2.2/download%60)

bors · 2023-09-18T19:41:24Z

⌛ Testing commit 9ac8b36 with merge cebb9cf...

bors · 2023-09-18T21:29:49Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing cebb9cf to master...

rust-timer · 2023-09-18T22:52:18Z

Finished benchmarking commit (cebb9cf): 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.5%	[0.5%, 0.5%]	1
Improvements ✅ (primary)	-5.0%	[-6.3%, -3.6%]	2
Improvements ✅ (secondary)	-6.1%	[-6.6%, -5.7%]	6
All ❌✅ (primary)	-5.0%	[-6.3%, -3.6%]	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.6%	[0.6%, 0.6%]	2
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-3.3%	[-4.8%, -1.5%]	4
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	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)	-5.7%	[-7.7%, -3.7%]	2
Improvements ✅ (secondary)	-3.9%	[-3.9%, -3.7%]	3
All ❌✅ (primary)	-5.7%	[-7.7%, -3.7%]	2

Binary size

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

Bootstrap: 633.591s -> 631.896s (-0.27%)
Artifact size: 318.00 MiB -> 317.84 MiB (-0.05%)

Propagate changes from rust-lang/rust#115748.

move required_consts check to general post-mono-check function This factors some code that is common between the interpreter and the codegen backends into shared helper functions. Also as a side-effect the interpreter now uses the same `eval` functions as everyone else to get the evaluated MIR constants. Also this is in preparation for another post-mono check that will be needed for (the current hackfix for) rust-lang#115709: ensuring that all locals are dynamically sized. I didn't expect this to change diagnostics, but it's just cycle errors that change. r? `@oli-obk`

rustbot assigned oli-obk Sep 11, 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Sep 11, 2023

RalfJung commented Sep 11, 2023

View reviewed changes