I'm okay with this change, but I would like @lcnr's opinion on whether this sort of whack-a-mole approach is sustainable. (I don't want to have an ever-growing number of is_trivial_const checks that eventually upsets the types team)

What are the requirements for something to be a trivial const, are they written down somewhere? E.g. is const FOO: Tait = 1; a trivial const?

I personally feel quite... unhappy about this change. @BoxyUwU has been changing lowering of some constants for const generics to never actually get lowered to HIR expression but instead eagerly convert them into type system constants. That means we don't have a body for them and don't even try to call any of the typeck/mir_borrowck queries.

Having both of these impls seems fairly bad to me. It seems like ideally there should only be one way to get constants without corresponding MIR body.

I don't have the time to engage with that area myself right now :x I think we should not have both systems and ideally you'd chat with @BoxyUwU to figure out whether you can unify the two somehow

Well, that's why I asked.

I agree that Boxy's approach seems better.

If you wanted to know the semantics I implemented, they're all in here: https://github.com/rust-lang/rust/blob/2fb805367dd3012ce5c50865d03c86e70bf10f0f/compiler/rustc_mir_transform/src/trivial_const.rs

E.g. is const FOO: Tait = 1; a trivial const?

so this ends up as a trivial const? we never check that the type of the const is not an opaque. This code would start to fail/misbehave with this PR because MIR borrowck doesn't return the defining use anymore, would it not (at least with the new solver)?

Would definitely be interested in chatting about things here 🤔 @saethlin if you wanna reach out on zulip at some point that'd be cool :)

Checking for progress. Did a conversation happen about this? What's the current status?

thanks for an update :)

E.g. is const FOO: Tait = 1; a trivial const?

so this ends up as a trivial const? we never check that the type of the const is not an opaque. This code would start to fail/misbehave with this PR because MIR borrowck doesn't return the defining use anymore, would it not (at least with the new solver)?

is this an issue with this PR?

E.g. is const FOO: Tait = 1; a trivial const?

so this ends up as a trivial const? we never check that the type of the const is not an opaque. This code would start to fail/misbehave with this PR because MIR borrowck doesn't return the defining use anymore, would it not (at least with the new solver)?

is this an issue with this PR?

I rebased the code with latest upstream.

For const FOO: Tait = 1; — the trivial_const function already has a body.has_opaque_types() check that returns None early, so it's not a trivial const and the borrowck will run.

Checking for progress. Did a conversation happen about this? What's the current status?

thanks for an update :)

I'm not sure about the status of lowering-level approach @BoxyUwU is working on, let's @BoxyUwU decide whether to close this PR?

r? lcnr

This optimization is unfortunately hard to unify with the work by Boxy. Given that we bail if there are any opaques,

For const FOO: Tait = 1; — the trivial_const function already has a body.has_opaque_types() check that returns None early, so it's not a trivial const and the borrowck will run.

I would expect that the current check isn't sufficient with the new solver here? Because in the new solver we actually normalize Tait to its underlying type during writeback, so the body doesn't reference the opaque itself, only when usng the fn_sig in borrowck do we actually constrain it.

Can you test it with -Znext-solver?

This has a borrowck error on nightly. Is it occurring in a trivial const?

struct Thing<'a>(&'a ());
impl<'a: 'static> Thing<'a> {
    const X: i32 = 1;
}
impl<'a> Thing<'a> {
    const C: i32 = Thing::<'a>::X;
}

This optimization is unfortunately hard to unify with the work by Boxy.

I agree. I started looking into what Boxy is doing, and I found it rather difficult to connect that logic to the optimization I was looking to implement.

@lcnr If you think the existing trivial_const logic is unsustainable, I trust your judgement and have no objection to reverting it.

Sorry, marked this as read without seeing your question. I do think this optimization has a very high complexity cost. Having a new kind of body and making sure it's always handled correctly feels brittle to me. I would prefer removing that change :/ I don't remember the perf improvements we get from this and maybe they justify the added complexity 🤔

I would expect that the current check isn't sufficient with the new solver here? Because in the new solver we actually normalize Tait to its underlying type during writeback, so the body doesn't reference the opaque itself, only when usng the fn_sig in borrowck do we actually constrain it.

Can you test it with -Znext-solver?

The post-merge perf run is here: #148040 (comment)

I think some level of complexity is warranted.

yeah :/ i feel conflicted here, which is why I am kind of avoiding this PR 😅

For const FOO: Tait = 1; — the trivial_const function already has a body.has_opaque_types() check that returns None early, so it's not a trivial const and the borrowck will run.

I would expect that the current check isn't sufficient with the new solver here? Because in the new solver we actually normalize Tait to its underlying type during writeback, so the body doesn't reference the opaque itself, only when usng the fn_sig in borrowck do we actually constrain it.

Can you test it with -Znext-solver?

we can fix this by instead checking whether the constant is the defining scope of any opaque. I guess I am fine with this PR if we add such a test and make sure it works

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

yeah :/ i feel conflicted here, which is why I am kind of avoiding this PR 😅

For const FOO: Tait = 1; — the trivial_const function already has a body.has_opaque_types() check that returns None early, so it's not a trivial const and the borrowck will run.

I would expect that the current check isn't sufficient with the new solver here? Because in the new solver we actually normalize Tait to its underlying type during writeback, so the body doesn't reference the opaque itself, only when usng the fn_sig in borrowck do we actually constrain it.
Can you test it with -Znext-solver?

we can fix this by instead checking whether the constant is the defining scope of any opaque. I guess I am fine with this PR if we add such a test and make sure it works

I changed the trivial-const check so that we do not treat a const as trivial if it is the defining scope of any opaque (tcx.opaque_types_defined_by(def)).

I also added a -Znext-solver regression test for this scenario. It fails without the new check and passes with it.

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@bors r=lcnr

📌 Commit 8e1f263 has been approved by lcnr

It is now in the queue for this repository.

☀️ Test successful - CI
Approved by: lcnr
Duration: 3h 16m 30s
Pushing f964de4 to main...

Comparing ffccab6 (parent) -> f964de4 (this PR)

Test differences

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard f964de49bcb561e5c6c725bb37201e11d852daf0 --output-dir test-dashboard

Job duration changes

1. dist-various-1: 57m 36s -> 1h 13m (+27.6%)
2. dist-x86_64-apple: 2h 18m -> 2h 56m (+27.2%)
3. dist-powerpc64le-linux-musl: 1h 19m -> 1h 34m (+18.9%)
4. dist-sparcv9-solaris: 1h 33m -> 1h 15m (-18.7%)
5. optional-x86_64-gnu-parallel-frontend: 2h 44m -> 2h 13m (-18.5%)
6. pr-check-1: 34m 45s -> 28m 26s (-18.2%)
7. dist-x86_64-msvc-alt: 2h 33m -> 2h 11m (-14.5%)
8. x86_64-rust-for-linux: 56m 15s -> 48m 40s (-13.5%)
9. pr-check-2: 45m 26s -> 39m 25s (-13.2%)
10. x86_64-gnu-nopt: 2h 32m -> 2h 12m (-13.2%)

Finished benchmarking commit (f964de4): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

Max RSS (memory usage)

Cycles

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 497.124s -> 508.875s (2.36%)
Artifact size: 395.05 MiB -> 395.04 MiB (-0.00%)