-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
merge DefKind::Coroutine
into Defkind::Closure
#118311
Conversation
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Or rather we need it sometimes, but it's not desirable to put that distinction into |
c98972c
to
f23befe
Compare
@bors r+ |
…llaumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#118296 (rustdoc: replace `elemIsInParent` with `Node.contains`) - rust-lang#118302 (Clean dead codes) - rust-lang#118311 (merge `DefKind::Coroutine` into `Defkind::Closure`) - rust-lang#118318 (Remove myself from users on vacation) r? `@ghost` `@rustbot` modify labels: rollup
let yield_ty = args.yield_ty(); | ||
let return_ty = args.return_ty(); | ||
(vec![coroutine_ty, args.resume_ty()], return_ty, Some(yield_ty)) | ||
} | ||
DefKind::Closure => { | ||
let closure_ty = tcx.type_of(def_id).instantiate_identity(); |
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.
The 2 Arms have the same structure. Can they be merged, and match on the type?
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.
Addressed in #118356.
hir_id: None, | ||
}; | ||
Some(coroutine_param) | ||
} | ||
DefKind::Closure => { | ||
let closure_ty = self.typeck_results.node_type(owner_id); |
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.
Likewise.
Rollup merge of rust-lang#118311 - bvanjoi:merge_coroutinue_into_closure, r=petrochenkov merge `DefKind::Coroutine` into `Defkind::Closure` Related to rust-lang#118188 We no longer need to be concerned about the precise type whether it's `DefKind::Closure` or `DefKind::Coroutine`. Furthermore, thanks for the great work done by `@petrochenkov` on investigating https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Why.20does.20it.20hang.20when.20querying.20.EF.BB.BF.60opt_def_kind.60.3F r? `@petrochenkov`
I plan to submit a PR attempting to reduce the calls to |
…e, r=<try> reduce the calls to `tcx.is_coroutine` This commit includes two changes: 1. It merges two arms to address the comment raised in rust-lang#118311 (comment) 2. It attempts to solve the performance regression discussed in rust-lang#118319 (comment) by reducing the number of calls to `tcx.is_coroutine`. r? `@cjgillot`
resolve: Feed the `def_kind` query immediately on `DefId` creation Before this PR the def kind query required building HIR for no good reason, with this PR def kinds are instead assigned immediately when `DefId`s are created. Some PRs previously refactored things to make all def kinds to be available early enough - rust-lang#118250, rust-lang#118272, rust-lang#118311.
…e_2, r=<try> aligns the behavior to that prior to rust-lang#118311 After rust-lang#118311, it seems that due to an oversight some alignments were unintentionally omitted, possibly leading the code into different branches. This PR attempts to restore those alignments and aims to fix the regression reported at rust-lang#118319 (comment)
Weird, the timing results for neither PR #118356 nor PR #118411 seems to indicate that they would fix the performance regression here. I would personally recommend that we put up a PR that reverts this one on its own, and benchmark that, just to confirm that that step would restore the performance loss. |
Sounds sensible. @bvanjoi, could you do that? Thanks. |
There are some conflicts with #118188, so I think they should be reverted together. |
…e_3, r=<try> revert for benchmark This PR reverts rust-lang#118311 and rust-lang#118188, with the intent to assess the performance impact brought about by rust-lang#118311
No, that's not a good idea for the experiment under discussion. Here's why: look at the performance results for #118188. There were a large number of performance improvements associated with that PR, so reverting PR #118188 will itself cause a separate set of regressions, and may well mask any improvement that reverting PR #118311 restores. |
I think I understood that relationship between #118188 and #118311. I was asking about, under our perf machinery, whether one can get the perf run that starts with (A) the state immediately prior to #118188 , and compares that to (B) that same state measured in A, but now with just #118311 reverted. I.e., go back in time to immediately before when #118188 landed, and do all analysis against that reference point. (But this is perhaps all moot at this point. If PR #118188 itself had more net benefit than the performance regression introduced by #118311, then we are probably best off just not worrying about this anymore, letting PR #118311 stand as is, and be happy that PR #118188 improved things.) |
…e_2, r=<try> aligns the behavior to that prior to rust-lang#118311 After rust-lang#118311, it seems that due to an oversight some alignments were unintentionally omitted, possibly leading the code into different branches. This PR attempts to restore those alignments and aims to fix the regression reported at rust-lang#118319 (comment)
…e_2, r=<try> aligns the behavior to that prior to rust-lang#118311 After rust-lang#118311, it seems that due to an oversight some alignments were unintentionally omitted, possibly leading the code into different branches. This PR attempts to restore those alignments and aims to fix the regression reported at rust-lang#118319 (comment)
Related to #118188
We no longer need to be concerned about the precise type whether it's
DefKind::Closure
orDefKind::Coroutine
.Furthermore, thanks for the great work done by @petrochenkov on investigating https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Why.20does.20it.20hang.20when.20querying.20.EF.BB.BF.60opt_def_kind.60.3F
r? @petrochenkov