merge `DefKind::Coroutine` into `Defkind::Closure` #118311

bvanjoi · 2023-11-26T11:50:03Z

Related to #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

rustbot · 2023-11-26T11:50:13Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

petrochenkov · 2023-11-26T12:19:11Z

We no longer need to be concerned about the precise type whether it's DefKind::Closure or DefKind::Coroutine.

Or rather we need it sometimes, but it's not desirable to put that distinction into DefKind because it cannot be determined early enough for #118188.

compiler/rustc_metadata/src/rmeta/encoder.rs

compiler/rustc_middle/src/ty/util.rs

petrochenkov · 2023-11-26T13:41:13Z

@bors r+

bors · 2023-11-26T13:41:16Z

📌 Commit f23befe has been approved by petrochenkov

It is now in the queue for this repository.

…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

cjgillot · 2023-11-26T18:21:13Z

compiler/rustc_mir_build/src/build/mod.rs

+            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();


The 2 Arms have the same structure. Can they be merged, and match on the type?

Addressed in #118356.

cjgillot · 2023-11-26T18:21:36Z

compiler/rustc_mir_build/src/thir/cx/mod.rs

+                    hir_id: None,
+                };
+                Some(coroutine_param)
+            }
            DefKind::Closure => {
                let closure_ty = self.typeck_results.node_type(owner_id);


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`

nnethercote · 2023-11-27T06:53:15Z

@bvanjoi Quite a few performance regressions, as seen here. Any idea where they might be from, and how to mitigate them?

bvanjoi · 2023-11-27T06:56:26Z

Any idea where they might be from, and how to mitigate them?

I plan to submit a PR attempting to reduce the calls to tcx.is_coroutine

…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)

pnkfelix · 2023-11-28T22:24:13Z

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.

nnethercote · 2023-11-29T02:17:16Z

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.

bvanjoi · 2023-11-29T04:53:33Z

Weird, the timing results for neither PR #118356 nor PR #118411 seems to indicate that they would fix the performance regression here.

Now, I'm suspecting that the effect of tcx.is_coroutine does not correspond to ExprKind::Closure(Closure { movability: Some(_), .. }) => DefKind::Coroutine.

bvanjoi · 2023-11-29T05:07:40Z

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.

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

pnkfelix · 2023-11-29T13:40:44Z

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.

There are some conflicts with #118188, so I think they should be reverted together.

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.

pnkfelix · 2023-11-29T13:43:25Z

Is there a way to get a perf run for a revert of PR #118311 atop the state of the the tree immediately prior to when PR #118188 landed?

petrochenkov · 2023-11-29T14:32:43Z

@pnkfelix
No, #118311 is a strict prerequisite for #118188.

pnkfelix · 2023-11-29T15:00:02Z

@pnkfelix No, #118311 is a strict prerequisite for #118188.

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)

rustbot assigned petrochenkov Nov 26, 2023

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 26, 2023

petrochenkov reviewed Nov 26, 2023

View reviewed changes

compiler/rustc_metadata/src/rmeta/encoder.rs Show resolved Hide resolved

compiler/rustc_middle/src/ty/util.rs Outdated Show resolved Hide resolved

petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2023

merge DefKind::Coroutine into DefKind::Closure

f23befe

bvanjoi force-pushed the merge_coroutinue_into_closure branch from c98972c to f23befe Compare November 26, 2023 13:06

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 26, 2023

petrochenkov mentioned this pull request Nov 26, 2023

resolve: Feed the def_kind query immediately on DefId creation #118188

Merged

GuillaumeGomez mentioned this pull request Nov 26, 2023

Rollup of 4 pull requests #118319

Merged

cjgillot reviewed Nov 26, 2023

View reviewed changes

bors merged commit c6d20d7 into rust-lang:master Nov 26, 2023
11 checks passed

rustbot added this to the 1.76.0 milestone Nov 26, 2023

bvanjoi deleted the merge_coroutinue_into_closure branch November 27, 2023 07:52

bvanjoi mentioned this pull request Nov 27, 2023

reduce the calls to tcx.is_coroutine #118356

Closed

bvanjoi added a commit to bvanjoi/rust that referenced this pull request Nov 28, 2023

aligns the behavior to that prior to rust-lang#118311

4307541

bvanjoi mentioned this pull request Nov 28, 2023

aligns the behavior to that prior to #118311 #118411

Closed

bvanjoi mentioned this pull request Nov 29, 2023

revert for benchmark #118436

Closed

bvanjoi added a commit to bvanjoi/rust that referenced this pull request Nov 29, 2023

aligns the behavior to that prior to rust-lang#118311

efa2fff

bvanjoi added a commit to bvanjoi/rust that referenced this pull request Jan 2, 2024

aligns the behavior to that prior to rust-lang#118311

4fda2d8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge `DefKind::Coroutine` into `Defkind::Closure` #118311

merge `DefKind::Coroutine` into `Defkind::Closure` #118311

bvanjoi commented Nov 26, 2023

rustbot commented Nov 26, 2023

petrochenkov commented Nov 26, 2023

petrochenkov commented Nov 26, 2023

bors commented Nov 26, 2023

cjgillot Nov 26, 2023

petrochenkov Nov 27, 2023

cjgillot Nov 26, 2023

nnethercote commented Nov 27, 2023 •

edited

Loading

bvanjoi commented Nov 27, 2023

pnkfelix commented Nov 28, 2023

nnethercote commented Nov 29, 2023

bvanjoi commented Nov 29, 2023

bvanjoi commented Nov 29, 2023

pnkfelix commented Nov 29, 2023 •

edited

Loading

pnkfelix commented Nov 29, 2023

petrochenkov commented Nov 29, 2023

pnkfelix commented Nov 29, 2023 •

edited

Loading

merge DefKind::Coroutine into Defkind::Closure #118311

merge DefKind::Coroutine into Defkind::Closure #118311

Conversation

bvanjoi commented Nov 26, 2023

rustbot commented Nov 26, 2023

petrochenkov commented Nov 26, 2023

petrochenkov commented Nov 26, 2023

bors commented Nov 26, 2023

cjgillot Nov 26, 2023

Choose a reason for hiding this comment

petrochenkov Nov 27, 2023

Choose a reason for hiding this comment

cjgillot Nov 26, 2023

Choose a reason for hiding this comment

nnethercote commented Nov 27, 2023 • edited Loading

bvanjoi commented Nov 27, 2023

pnkfelix commented Nov 28, 2023

nnethercote commented Nov 29, 2023

bvanjoi commented Nov 29, 2023

bvanjoi commented Nov 29, 2023

pnkfelix commented Nov 29, 2023 • edited Loading

pnkfelix commented Nov 29, 2023

petrochenkov commented Nov 29, 2023

pnkfelix commented Nov 29, 2023 • edited Loading

merge `DefKind::Coroutine` into `Defkind::Closure` #118311

merge `DefKind::Coroutine` into `Defkind::Closure` #118311

nnethercote commented Nov 27, 2023 •

edited

Loading

pnkfelix commented Nov 29, 2023 •

edited

Loading

pnkfelix commented Nov 29, 2023 •

edited

Loading