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

subst -> instantiate #116144

Merged
merged 1 commit into from
Sep 26, 2023
Merged

subst -> instantiate #116144

merged 1 commit into from
Sep 26, 2023

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Sep 25, 2023

continues #110793, there are still quite a few uses of subst and substitute, but changing them all in the same PR was a bit too much, so I've stopped here for now.

@rustbot
Copy link
Collaborator

rustbot commented Sep 25, 2023

r? @petrochenkov

(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 Sep 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 25, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@lcnr
Copy link
Contributor Author

lcnr commented Sep 25, 2023

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned petrochenkov Sep 25, 2023
@@ -619,7 +619,7 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
// programs, so we need to use delay_span_bug here. See #82126.
self.infcx.tcx.sess.delay_span_bug(
hir_arg.span(),
format!("unmatched subst and hir arg: found {kind:?} vs {hir_arg:?}"),
format!("unmatched arg and hir arg: found {kind:?} vs {hir_arg:?}"),
Copy link
Member

Choose a reason for hiding this comment

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

"arg and hir arg" -- maybe "generic param and hir arg"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, it is a ty::GenericArg and a hir generic arg, the ty generic arg could be something like 3 which is not a param 🤔 the comment is somewhat :/ rn though

@@ -427,7 +427,7 @@ impl<'tcx> dyn AstConv<'tcx> + '_ {
let bound_vars = tcx.late_bound_vars(binding.hir_id);
ty::Binder::bind_with_vars(subst_output, bound_vars)
} else {
// Include substitutions for generic parameters of associated types
// Include instantiations of the generic parameters of associated types
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this comment doesn't make sense.

@@ -569,7 +569,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
) -> Result<T, ErrorHandled> {
frame
.instance
.try_subst_mir_and_normalize_erasing_regions(
.try_instantiate_mir_and_normalize_erasing_regions(
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I found the previous term much clearer (it's a generic function and we are substituting in concrete values)... but I guess that ship has sailed so it probably makes sense to make this consistent.

The function this is in is called subst_from_frame_and_normalize_erasing_regions. So do you suggest that should be renamed to instantiate_from_frame_and_normalize_erasing_regions or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function this is in is called subst_from_frame_and_normalize_erasing_regions. So do you suggest that should be renamed to instantiate_from_frame_and_normalize_erasing_regions or so?

yeah, it does feel a bit verbose though 😅 🤷

@rustbot
Copy link
Collaborator

rustbot commented Sep 26, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@oli-obk
Copy link
Contributor

oli-obk commented Sep 26, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 26, 2023

📌 Commit 3c52a3e 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 Sep 26, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 26, 2023
subst -> instantiate

continues rust-lang#110793, there are still quite a few uses of `subst` and `substitute`, but changing them all in the same PR was a bit too much, so I've stopped here for now.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 26, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#116099 (Add regression test for issue rust-lang#79865)
 - rust-lang#116102 (Correct codegen of `ConstValue::Indirect` scalar and scalar pair)
 - rust-lang#116131 (Rename `cold_path` to `outline`)
 - rust-lang#116144 (subst -> instantiate)
 - rust-lang#116151 (Fix typo in rustdoc unstable features doc)
 - rust-lang#116153 (Update books)
 - rust-lang#116162 (Gate and validate `#[rustc_safe_intrinsic]`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 26, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#116099 (Add regression test for issue rust-lang#79865)
 - rust-lang#116102 (Correct codegen of `ConstValue::Indirect` scalar and scalar pair)
 - rust-lang#116131 (Rename `cold_path` to `outline`)
 - rust-lang#116144 (subst -> instantiate)
 - rust-lang#116151 (Fix typo in rustdoc unstable features doc)
 - rust-lang#116153 (Update books)
 - rust-lang#116162 (Gate and validate `#[rustc_safe_intrinsic]`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Sep 26, 2023

⌛ Testing commit 3c52a3e with merge 5ae769f...

@bors
Copy link
Contributor

bors commented Sep 26, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 5ae769f to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Sep 26, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 5ae769f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 26, 2023
@bors bors merged commit 5ae769f into rust-lang:master Sep 26, 2023
12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 26, 2023
This was referenced Sep 26, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5ae769f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.5% [2.4%, 2.6%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 633.132s -> 631.857s (-0.20%)
Artifact size: 317.17 MiB -> 317.04 MiB (-0.04%)

@lcnr lcnr deleted the subst-less branch September 28, 2023 11:57
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 6, 2023
subst -> instantiate

continues rust-lang#110793, there are still quite a few uses of `subst` and `substitute`, but changing them all in the same PR was a bit too much, so I've stopped here for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

8 participants