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.

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

Add eval hack in super_relate_consts back #103279

Merged
merged 2 commits into from
Oct 26, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Oct 20, 2022

Partially reverts 01adb7e.

This extra eval call still needs to happen, for example, in normalize_param_env_or_error when a param-env predicate has an unnormalized constant, since the param-env candidates never get normalized during candidate assembly (everywhere else we can assume that they are normalized fully).

r? @lcnr, though I feel like I've assigned quite a few PRs to you in the last few days, so feel free to reassign to someone else familiar with this code if you're busy!

cc #103243 (fixes the issue, but don't want to auto-close that until a backport is performed).

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 20, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2022
@compiler-errors compiler-errors changed the title Add normalize hack back Add normalize hack in super_relate_consts back Oct 20, 2022
@compiler-errors compiler-errors changed the title Add normalize hack in super_relate_consts back Add eval hack in super_relate_consts back Oct 20, 2022
@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 20, 2022
@Mark-Simulacrum
Copy link
Member

Beta nominating (fixes regression).

@rustbot
Copy link
Collaborator

rustbot commented Oct 20, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@compiler-errors
Copy link
Member Author

compiler-errors commented Oct 20, 2022

The second commit in this stack suppresses a few triggered debug assertions (

debug_assert_eq!(trait_ref, tcx.normalize_erasing_regions(param_env, trait_ref));
) discovered after fixing this issue.

These don't necessarily need to be backported since the triggered assertion is a debug_assert_eq!.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

nits, then r=me

@@ -587,7 +587,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
mir::ConstantKind::Val(val, ty) => self.const_val_to_op(*val, *ty, layout),
mir::ConstantKind::Unevaluated(uv, _) => {
let instance = self.resolve(uv.def, uv.substs)?;
let substs = self.tcx.normalize_erasing_regions(self.param_env, uv.substs);
Copy link
Contributor

Choose a reason for hiding this comment

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

that shouldn't be necessary because the const should already be normalized here. You could add a debug assert that this normalize call would be a noop though.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary, because we still triggered a debug assertion without it. Not needed for the backport, per se.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, ok 🤔 then we should normalize further above, can keep this with a FIXME for now ^^

Copy link
Member Author

@compiler-errors compiler-errors Oct 20, 2022

Choose a reason for hiding this comment

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

What do you mean further above? The unevaluated constants are the only ones that need normalization, right? edit: nvm i see

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, to be more clear: this normalization is needed because of the const prop lint in mir_drops_elaborated_and_const_checked, which happens before optimized MIR. Only after optimizing the MIR can we guarantee that the RevealAll pass has happened and that the body's constants are normalized, so any call to resolve before that needs to be manually normalized.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, the const prop lint should still normalize itself if it uses Reveal::All

Copy link
Contributor

Choose a reason for hiding this comment

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

to not block anything here: can you add a FIXME and then we merge this as is for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a FIXME now

let substs = self.typeck_results.node_substs(id);
let substs = self
.tcx
.normalize_erasing_regions(param_env_reveal_all, self.typeck_results.node_substs(id));
Copy link
Contributor

@lcnr lcnr Oct 20, 2022

Choose a reason for hiding this comment

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

that normalize call seems correct and useful. Considering that we want to backport this, is this call necessary to fix these regressions? I guess I am fine with keeping this even if not 🤷

Comment on lines 599 to 602
// HACK(consts): We still need to eagerly evaluate consts when
// relating them because during `normalize_param_env_or_error`,
// we may relate an evaluated constant in a obligation against
// an unnormalized (i.e. unevaluated) const in the param-env.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// HACK(consts): We still need to eagerly evaluate consts when
// relating them because during `normalize_param_env_or_error`,
// we may relate an evaluated constant in a obligation against
// an unnormalized (i.e. unevaluated) const in the param-env.
// HACK(const_generics): We still need to eagerly evaluate consts when
// relating them because during `normalize_param_env_or_error`,
// we may relate an evaluated constant in a obligation against
// an unnormalized (i.e. unevaluated) const in the param-env.
// FIXME(generic_const_exprs): Once we always lazily unify unevaluated constants
// these `eval` calls can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's also probably broken with feature(generic_const_exprs) because with that constants can refer to higher ranked lifetimes and some type relations don't actually convert bound variables to placeholders/infer when unifying, so this would cause an ICE.

Can you change this to only run if gce is not enabled.

@lcnr
Copy link
Contributor

lcnr commented Oct 20, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 20, 2022
@bors
Copy link
Contributor

bors commented Oct 20, 2022

⌛ Trying commit 66a57355761afc93595f4bbc6ed50330f408a2cd with merge 165ab7a9eeecf3abb7cf89dd992dd4a1d333ce12...

@bors
Copy link
Contributor

bors commented Oct 20, 2022

☀️ Try build successful - checks-actions
Build commit: 165ab7a9eeecf3abb7cf89dd992dd4a1d333ce12 (165ab7a9eeecf3abb7cf89dd992dd4a1d333ce12)

@rust-timer
Copy link
Collaborator

Queued 165ab7a9eeecf3abb7cf89dd992dd4a1d333ce12 with parent ebdde35, future comparison URL.

@apiraino
Copy link
Contributor

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 20, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (165ab7a9eeecf3abb7cf89dd992dd4a1d333ce12): comparison URL.

Overall result: ❌ regressions - 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-review -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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.8% [0.7%, 1.0%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

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

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.6% [2.3%, 2.8%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 20, 2022
@compiler-errors
Copy link
Member Author

@bors r=lcnr addressed the outstanding nits

@bors
Copy link
Contributor

bors commented Oct 21, 2022

📌 Commit e590835ef69bf5696ca3cdb4708fad409c401c65 has been approved by lcnr

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 Oct 21, 2022
@bors
Copy link
Contributor

bors commented Oct 23, 2022

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 23, 2022
@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Oct 23, 2022

📌 Commit 6e6fe30 has been approved by lcnr

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 26, 2022

⌛ Testing commit 6e6fe30 with merge d49e7e7...

@bors
Copy link
Contributor

bors commented Oct 26, 2022

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing d49e7e7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 26, 2022
@bors bors merged commit d49e7e7 into rust-lang:master Oct 26, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 26, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d49e7e7): comparison URL.

Overall result: ❌ regressions - 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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.3% [1.3%, 1.4%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
7.0% [7.0%, 7.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

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

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 29, 2022
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.66.0, 1.65.0 Oct 29, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 29, 2022
…mulacrum

[beta] backport rollup

* poll_fn and Unpin: fix pinning rust-lang#102737
* Support raw-dylib functions being used inside inlined functions rust-lang#102988
* Fix line numbers for MIR inlined code rust-lang#103071
* Add architectures to fn create_object_file rust-lang#103240
* Add eval hack in super_relate_consts back rust-lang#103279
* Mark std::os::wasi::io::AsFd etc. as stable. rust-lang#103308
* Truncate thread names on Linux and Apple targets rust-lang#103379
* Do not consider repeated lifetime params for elision. rust-lang#103450
* rustdoc: add missing URL redirect rust-lang#103588
* Remove commit_if_ok probe from NLL type relation rust-lang#103601

Also includes a copy of the release notes.

r? `@ghost`
@compiler-errors compiler-errors deleted the normalize-hack-back branch November 2, 2022 02:53
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
… r=lcnr

Add eval hack in `super_relate_consts` back

Partially reverts 01adb7e.

This extra eval call *still* needs to happen, for example, in `normalize_param_env_or_error` when a param-env predicate has an unnormalized constant, since the param-env candidates never get normalized during candidate assembly (everywhere else we can assume that they are normalized fully).

r? `@lcnr,` though I feel like I've assigned quite a few PRs to you in the last few days, so feel free to reassign to someone else familiar with this code if you're busy!

cc rust-lang#103243 (fixes the issue, but don't want to auto-close that until a backport is performed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants