-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Rename interner funcs #108250
Rename interner funcs #108250
Conversation
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt The Miri subtree was changed cc @rust-lang/miri Some changes occured in cc @BoxyUwU Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in src/tools/rust-analyzer cc @rust-lang/wg-rls-2 Some changes occurred in compiler/rustc_codegen_gcc cc @antoyo Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
I'm not expecting perf changes, but let's check. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 5bd40e576311fc2aba937c11c00fda332233bb8d with merge 424f380de9b55b49897023b94fb8dc2a55e2454e... |
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.
One nit, mk_substs(&[])
can just be List::empty()
, right?
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, nits or not.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (424f380de9b55b49897023b94fb8dc2a55e2454e): comparison URL. Overall result: ❌ regressions - 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. |
#108112 renamed a bunch of mk_* to intern_*. Why rename them back again? |
Currently, for many cases there is a #108112 changed a number of This PR renames the functions as |
5bd40e5
to
a5181d8
Compare
Lots of small regression in incremental builds is annoying. No obvious reason why, and I did some local analysis and it just seems to be due to minor variations in codegen triggered by nothing in particular. Let's try another perf run: @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit a5181d821f665c20a71003d4310e3d93ba055901 with merge 33a46a28002e0a45db67472e697d8ad77d84914f... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (33a46a28002e0a45db67472e697d8ad77d84914f): comparison URL. Overall result: ❌ regressions - 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. |
All the slice interners have a wrapper that handles the empty slice case. We can instead handle this in the `slice_interners!` macro, avoiding the need for most of the wrappers, and allowing the interner functions to be renamed from `_intern_foos` to `intern_foos`. The two exceptions: - intern_predicates: I kept this wrapper because there's a FIXME comment about a possible future change. - intern_poly_existential_predicates: I kept this wrapper because it asserts that the slice is empty and sorted.
(This is a large commit. The changes to `compiler/rustc_middle/src/ty/context.rs` are the most important ones.) The current naming scheme is a mess, with a mix of `_intern_`, `intern_` and `mk_` prefixes, with little consistency. In particular, in many cases it's easy to use an iterator interner when a (preferable) slice interner is available. The guiding principles of the new naming system: - No `_intern_` prefixes. - The `intern_` prefix is for internal operations. - The `mk_` prefix is for external operations. - For cases where there is a slice interner and an iterator interner, the former is `mk_foo` and the latter is `mk_foo_from_iter`. Also, `slice_interners!` and `direct_interners!` can now be `pub` or non-`pub`, which helps enforce the internal/external operations division. It's not perfect, but I think it's a clear improvement. The following lists show everything that was renamed. slice_interners - const_list - mk_const_list -> mk_const_list_from_iter - intern_const_list -> mk_const_list - substs - mk_substs -> mk_substs_from_iter - intern_substs -> mk_substs - check_substs -> check_and_mk_substs (this is a weird one) - canonical_var_infos - intern_canonical_var_infos -> mk_canonical_var_infos - poly_existential_predicates - mk_poly_existential_predicates -> mk_poly_existential_predicates_from_iter - intern_poly_existential_predicates -> mk_poly_existential_predicates - _intern_poly_existential_predicates -> intern_poly_existential_predicates - predicates - mk_predicates -> mk_predicates_from_iter - intern_predicates -> mk_predicates - _intern_predicates -> intern_predicates - projs - intern_projs -> mk_projs - place_elems - mk_place_elems -> mk_place_elems_from_iter - intern_place_elems -> mk_place_elems - bound_variable_kinds - mk_bound_variable_kinds -> mk_bound_variable_kinds_from_iter - intern_bound_variable_kinds -> mk_bound_variable_kinds direct_interners - region - intern_region (unchanged) - const - mk_const_internal -> intern_const - const_allocation - intern_const_alloc -> mk_const_alloc - layout - intern_layout -> mk_layout - adt_def - intern_adt_def -> mk_adt_def_from_data (unusual case, hard to avoid) - alloc_adt_def(!) -> mk_adt_def - external_constraints - intern_external_constraints -> mk_external_constraints Other - type_list - mk_type_list -> mk_type_list_from_iter - intern_type_list -> mk_type_list - tup - mk_tup -> mk_tup_from_iter - intern_tup -> mk_tup
To discourage accidental use -- there are more specific `mk_*` functions for all `Ty` and `Region` kinds.
It's missing, and is useful in two places.
da815d9
to
08f28f9
Compare
I rebased again. @bors r=compiler-errors |
☀️ Test successful - checks-actions |
Finished benchmarking commit (dcca6a3): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
|
Remove some superfluous type parameters from layout.rs rust-lang/rust#107163 Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 Unify validity checks into a single query rust-lang/rust#108364 s/eval_usize/eval_target_usize/ for clarity rust-lang/rust#108029 Use target instead of machine for mir interpreter integer handling rust-lang/rust#108047 Switch to EarlyBinder for type_of query rust-lang/rust#107753 Rename interner funcs rust-lang/rust#108250 Optimize mk_region rust-lang/rust#108020 Clarify iterator interners rust-lang/rust#108112
- Remove some superfluous type parameters from layout.rs rust-lang/rust#107163 - Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 - Unify validity checks into a single query rust-lang/rust#108364 - s/eval_usize/eval_target_usize/ for clarity rust-lang/rust#108029 - Use target instead of machine for mir interpreter integer handling rust-lang/rust#108047 - Switch to EarlyBinder for type_of query rust-lang/rust#107753 - Rename interner funcs rust-lang/rust#108250 - Optimize mk_region rust-lang/rust#108020 - Clarify iterator interners rust-lang/rust#108112
- Remove some superfluous type parameters from layout.rs rust-lang/rust#107163 - Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 - Unify validity checks into a single query rust-lang/rust#108364 - s/eval_usize/eval_target_usize/ for clarity rust-lang/rust#108029 - Use target instead of machine for mir interpreter integer handling rust-lang/rust#108047 - Switch to EarlyBinder for type_of query rust-lang/rust#107753 - Rename interner funcs rust-lang/rust#108250 - Optimize mk_region rust-lang/rust#108020 - Clarify iterator interners rust-lang/rust#108112
- Remove some superfluous type parameters from layout.rs rust-lang/rust#107163 - Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 - Unify validity checks into a single query rust-lang/rust#108364 - s/eval_usize/eval_target_usize/ for clarity rust-lang/rust#108029 - Use target instead of machine for mir interpreter integer handling rust-lang/rust#108047 - Switch to EarlyBinder for type_of query rust-lang/rust#107753 - Rename interner funcs rust-lang/rust#108250 - Optimize mk_region rust-lang/rust#108020 - Clarify iterator interners rust-lang/rust#108112
- Remove some superfluous type parameters from layout.rs rust-lang/rust#107163 - Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 - Unify validity checks into a single query rust-lang/rust#108364 - s/eval_usize/eval_target_usize/ for clarity rust-lang/rust#108029 - Use target instead of machine for mir interpreter integer handling rust-lang/rust#108047 - Switch to EarlyBinder for type_of query rust-lang/rust#107753 - Rename interner funcs rust-lang/rust#108250 - Optimize mk_region rust-lang/rust#108020 - Clarify iterator interners rust-lang/rust#108112
- Remove some superfluous type parameters from layout.rs rust-lang/rust#107163 - Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 - Unify validity checks into a single query rust-lang/rust#108364 - s/eval_usize/eval_target_usize/ for clarity rust-lang/rust#108029 - Use target instead of machine for mir interpreter integer handling rust-lang/rust#108047 - Switch to EarlyBinder for type_of query rust-lang/rust#107753 - Rename interner funcs rust-lang/rust#108250 - Optimize mk_region rust-lang/rust#108020 - Clarify iterator interners rust-lang/rust#108112
- Introduce -Zterminal-urls to use OSC8 for error codes rust-lang/rust#107838 - Unify validity checks into a single query rust-lang/rust#108364 - Rename interner funcs rust-lang/rust#108250 - Optimize mk_region rust-lang/rust#108020 - Clarify iterator interners rust-lang/rust#108112
This PR cleans up some inconsistencies in interner naming.
Best reviewed one commit at a time.
r? @compiler-errors