Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure param-env is const before calling eval_to_valtree #105847

Merged
merged 1 commit into from
Dec 23, 2022
Merged

Ensure param-env is const before calling eval_to_valtree #105847

merged 1 commit into from
Dec 23, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Dec 17, 2022 •
edited
Loading

Other queries call ParamEnv::with_const inside of the query itself (e.g. const_eval_global_id_for_typeck), so this could alternatively be moved into the provider of eval_to_valtree instead. I don't have a particularly strong opinion, though theoretically caching is better if we make the query keys more constrained.

I'm not exactly sure how this is an effect of the -Zmir-opt-level=3 flag. Maybe something about the inliner causes us to inline an unevaluated const into a body where it can be evaluated, but where it has not yet been normalized.

This seems likely, since we're inlining from_fn_1::<{ N / 2 }, _> in from_fn_2, which means that we will need to evaluate that constant during the const prop pass after inlining.

Fixes #104396

@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2022

r? @estebank

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

@rustbot 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 Dec 17, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

oli-obk
oli-obk reviewed Dec 18, 2022
View reviewed changes
compiler/rustc_const_eval/src/interpret/operand.rs
self.ctfe_query(span, |tcx| tcx.eval_to_valtree(self.param_env.and(cid)))?
.unwrap_or_else(|| bug!("unable to create ValTree for {uv:?}"))
self.ctfe_query(span, |tcx| {
tcx.eval_to_valtree(self.param_env.with_const().and(cid))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the eval_to_valtree query is just missing a remap_constness

rust/compiler/rustc_middle/src/query/mod.rs

Line 1013 in cbc70ff

query eval_to_valtree(

I'll be so happy when the constness field is finally gone

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk doesn't remap_env_constness set the param-env constness to not const?

Also, none of the other eval-like queries use remap_env_constness.

@compiler-errors
Copy link
Member Author

r? @oli-obk
@rustbot author

@rustbot rustbot assigned oli-obk and unassigned estebank Dec 19, 2022
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2022
@compiler-errors
Copy link
Member Author

I don't think I should be using remap_env_constness here -- it doesn't fix the issue.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 22, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Dec 22, 2022

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Dec 22, 2022

📌 Commit c1181e1 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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 Dec 22, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 22, 2022
@matthiaskrgr
Rollup merge of rust-lang#105847 - compiler-errors:issue-104396, r=ol…
2020c09 
…i-obk

Ensure param-env is const before calling `eval_to_valtree`

Other queries call `ParamEnv::with_const` *inside* of the query itself (e.g. `const_eval_global_id_for_typeck`), so this could alternatively be moved into the provider of `eval_to_valtree` instead. I don't have a particularly strong opinion, though *theoretically* caching is better if we make the query keys more constrained.

I'm not exactly sure how this is an effect of the `-Zmir-opt-level=3` flag. Maybe something about the inliner causes us to inline an unevaluated const into a body where it can be evaluated, but where it has not yet been normalized.

This seems likely, since we're inlining `from_fn_1::<{ N / 2 }, _>` in `from_fn_2`, which means that we will need to evaluate that constant during the const prop pass after inlining.

Fixes rust-lang#104396
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 22, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 22, 2022
@matthiaskrgr
Rollup merge of rust-lang#105847 - compiler-errors:issue-104396, r=ol…
e627938 
…i-obk

Ensure param-env is const before calling `eval_to_valtree`

Other queries call `ParamEnv::with_const` *inside* of the query itself (e.g. `const_eval_global_id_for_typeck`), so this could alternatively be moved into the provider of `eval_to_valtree` instead. I don't have a particularly strong opinion, though *theoretically* caching is better if we make the query keys more constrained.

I'm not exactly sure how this is an effect of the `-Zmir-opt-level=3` flag. Maybe something about the inliner causes us to inline an unevaluated const into a body where it can be evaluated, but where it has not yet been normalized.

This seems likely, since we're inlining `from_fn_1::<{ N / 2 }, _>` in `from_fn_2`, which means that we will need to evaluate that constant during the const prop pass after inlining.

Fixes rust-lang#104396
This was referenced Dec 22, 2022
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 22, 2022
@bors
Auto merge of rust-lang#106054 - matthiaskrgr:rollup-38epsfh, r=matth…
8a97b48 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#105567 (KCFI test: Also support LLVM 16 output)
 - rust-lang#105847 (Ensure param-env is const before calling `eval_to_valtree`)
 - rust-lang#105983 (Add a missing early return in drop tracking `handle_uninhabited_return`)
 - rust-lang#106027 (rustdoc: simplify CSS and DOM for more-scraped-examples)
 - rust-lang#106035 (Migrate search tab title color to CSS variable)
 - rust-lang#106037 (Add regression test for rust-lang#94293)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 548d49c into rust-lang:master Dec 23, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 23, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@matthiaskrgr
Rollup merge of rust-lang#105847 - compiler-errors:issue-104396, r=ol…
da5e686 
…i-obk

Ensure param-env is const before calling `eval_to_valtree`

Other queries call `ParamEnv::with_const` *inside* of the query itself (e.g. `const_eval_global_id_for_typeck`), so this could alternatively be moved into the provider of `eval_to_valtree` instead. I don't have a particularly strong opinion, though *theoretically* caching is better if we make the query keys more constrained.

I'm not exactly sure how this is an effect of the `-Zmir-opt-level=3` flag. Maybe something about the inliner causes us to inline an unevaluated const into a body where it can be evaluated, but where it has not yet been normalized.

This seems likely, since we're inlining `from_fn_1::<{ N / 2 }, _>` in `from_fn_2`, which means that we will need to evaluate that constant during the const prop pass after inlining.

Fixes rust-lang#104396
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#106054 - matthiaskrgr:rollup-38epsfh, r=matth…
7ac9439 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#105567 (KCFI test: Also support LLVM 16 output)
 - rust-lang#105847 (Ensure param-env is const before calling `eval_to_valtree`)
 - rust-lang#105983 (Add a missing early return in drop tracking `handle_uninhabited_return`)
 - rust-lang#106027 (rustdoc: simplify CSS and DOM for more-scraped-examples)
 - rust-lang#106035 (Migrate search tab title color to CSS variable)
 - rust-lang#106037 (Add regression test for rust-lang#94293)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@clubby789 clubby789 mentioned this pull request May 12, 2023
Closed
@compiler-errors compiler-errors deleted the issue-104396 branch August 11, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.68.0
5 participants
@compiler-errors @rustbot @oli-obk @bors @estebank