-
Notifications
You must be signed in to change notification settings - Fork 13.3k
interpret: reduce usage of TypingEnv::fully_monomorphized #134058
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
Conversation
Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
FYI |
@bors try |
This comment has been minimized.
This comment has been minimized.
I guess the question is whether this is actually more correct. We know the DefId we are resolving here, and we know it has to be a monomorphic function. So it should be equivalent, right? |
interpret: reduce usage of TypingEnv::fully_monomorphized r? `@lcnr`
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
let ty = tcx.ty_ordering_enum(None); | ||
let layout = | ||
tcx.layout_of(ty::TypingEnv::fully_monomorphized().as_query_input(ty)).unwrap(); | ||
Self::from_scalar(Scalar::from_i8(c as i8), layout) | ||
} | ||
|
||
pub fn from_pair(a: Self, b: Self, tcx: TyCtxt<'tcx>) -> Self { | ||
let layout = tcx | ||
pub fn from_pair(a: Self, b: Self, cx: &(impl HasTypingEnv<'tcx> + HasTyCtxt<'tcx>)) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess the current assumption is that scalar pairs are always monomorphic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently only used for the return values of operations like AddWithOverflow, which is always monomorphic. But seems better to not implicit assume this, and the new version is actually more ergonomic to use than the old one...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me on the changes assuming that perf is green
Looks like the build worked except for uploading some metrics somewhere? |
interpret: reduce usage of TypingEnv::fully_monomorphized r? `@lcnr`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (50c90c7): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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 Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 1.4%, secondary 4.3%)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.
CyclesResults (primary 3.9%)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.
Binary sizeResults (secondary -0.0%)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.
Bootstrap: 767.262s -> 765.905s (-0.18%) |
@bors r=lcnr |
interpret: reduce usage of TypingEnv::fully_monomorphized r? `@lcnr`
@bors retry |
interpret: reduce usage of TypingEnv::fully_monomorphized r? `@lcnr`
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
Spurious, what the-. @bors retry |
interpret: reduce usage of TypingEnv::fully_monomorphized r? `@lcnr`
@bors retry rollup=always |
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#133900 (Advent of `tests/ui` (misc cleanups and improvements) [1/N]) - rust-lang#133937 (Keep track of parse errors in `mod`s and don't emit resolve errors for paths involving them) - rust-lang#133938 (`rustc_mir_dataflow` cleanups, including some renamings) - rust-lang#134058 (interpret: reduce usage of TypingEnv::fully_monomorphized) - rust-lang#134130 (Stop using driver queries in the public API) - rust-lang#134140 (Add AST support for unsafe binders) - rust-lang#134229 (Fix typos in docs on provenance) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#133900 (Advent of `tests/ui` (misc cleanups and improvements) [1/N]) - rust-lang#133937 (Keep track of parse errors in `mod`s and don't emit resolve errors for paths involving them) - rust-lang#133938 (`rustc_mir_dataflow` cleanups, including some renamings) - rust-lang#134058 (interpret: reduce usage of TypingEnv::fully_monomorphized) - rust-lang#134130 (Stop using driver queries in the public API) - rust-lang#134140 (Add AST support for unsafe binders) - rust-lang#134229 (Fix typos in docs on provenance) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#134058 - RalfJung:interpret-typing-env, r=lcnr interpret: reduce usage of TypingEnv::fully_monomorphized r? `@lcnr`
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#133900 (Advent of `tests/ui` (misc cleanups and improvements) [1/N]) - rust-lang#133937 (Keep track of parse errors in `mod`s and don't emit resolve errors for paths involving them) - rust-lang#133938 (`rustc_mir_dataflow` cleanups, including some renamings) - rust-lang#134058 (interpret: reduce usage of TypingEnv::fully_monomorphized) - rust-lang#134130 (Stop using driver queries in the public API) - rust-lang#134140 (Add AST support for unsafe binders) - rust-lang#134229 (Fix typos in docs on provenance) r? `@ghost` `@rustbot` modify labels: rollup
r? @lcnr