Make `TyKind` `Copy` and change `ty.kind()` to return `TyKind` #77482

LeSeulArtichaut · 2020-10-03T12:25:42Z

Implements T-compiler MCP rust-lang/compiler-team#363.
This should be perf-tested.

r? @lcnr cc @nikomatsakis

rust-highfive · 2020-10-03T12:25:46Z

Some changes occurred in intra-doc-links.

cc @jyn514

lcnr · 2020-10-03T12:30:49Z

❤️

@bors try @rust-timer perf

bors · 2020-10-03T12:31:02Z

⌛ Trying commit 5268ae7 with merge a9b36da2412359e22b389fab4829683fdbe81dbb...

lcnr

some small nits, otherwise this is a lot cleaner than before. Thanks!

compiler/rustc_middle/src/ty/codec.rs

compiler/rustc_middle/src/ty/error.rs

lcnr · 2020-10-03T12:41:04Z

compiler/rustc_middle/src/ty/error.rs

@@ -575,7 +575,7 @@ impl<T> Trait<T> for X {
                // This will also work for `impl Trait`.
                let def_id = if let ty::Param(param_ty) = proj_ty.self_ty().kind() {
                    let generics = self.generics_of(body_owner_def_id);
-                    generics.type_param(param_ty, self).def_id
+                    generics.type_param(&param_ty, self).def_id


Probably want to instead take ParamTy by value in type_param, but using a reference is fine for now here

compiler/rustc_middle/src/ty/print/pretty.rs

compiler/rustc_middle/src/ty/relate.rs

lcnr · 2020-10-03T12:46:05Z

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

@@ -347,7 +347,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                        "`Self`",
                        err,
                        None,
-                        projection,
+                        projection.as_ref(),


followup: probably take Option<ProjectionTy> in suggest_restriction

lcnr · 2020-10-03T12:50:56Z

compiler/rustc_typeck/src/check/op.rs

-                        *r_ty.kind() == Str || is_std_string(r_ty) ||
+            (Ref(_, l_ty, _), Ref(_, r_ty, _)) // &str or &String + &str, &String or &&str
+                if (l_ty.kind() == Str || is_std_string(l_ty)) && (
+                        r_ty.kind() == Str || is_std_string(r_ty) ||
                        &format!("{:?}", rhs_ty) == "&&str"


unrelated, but we really shouldn't compare on format!("{:?}", rhs_ty) here

lcnr · 2020-10-03T12:55:26Z

used the wrong rust-timer command

edit: and again

@rust-timer queue

rust-timer · 2020-10-03T12:55:48Z

Awaiting bors try build completion

bors · 2020-10-03T13:15:54Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: a9b36da2412359e22b389fab4829683fdbe81dbb (a9b36da2412359e22b389fab4829683fdbe81dbb)

rust-timer · 2020-10-03T13:15:57Z

Queued a9b36da2412359e22b389fab4829683fdbe81dbb with parent 6f56fbd, future comparison URL.

rust-timer · 2020-10-03T14:55:05Z

Finished benchmarking try commit (a9b36da2412359e22b389fab4829683fdbe81dbb): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

LeSeulArtichaut · 2020-10-03T14:58:57Z

Also cc @eddyb in case you're interested in the perf results

lcnr · 2020-10-03T15:41:52Z

not sure about perf, cycles looks quite positive, wall-time is probably about neutral all considered and instructions got worse.

I would have expected that we mostly just instantly match on ty.kind() which requires loading it from memory anyways, so I don't fully get why this increases the instruction count here.

lcnr · 2020-10-03T15:52:05Z

I think using ref patterns here ends up causing worse codegen when returning TyKind by value in kind.

So we probably get better perf - and cleaner code - if we eliminate these.

experimented in https://godbolt.org/z/o3bsM3

lcnr · 2020-10-03T16:41:48Z

compiler/rustc_middle/src/ty/relate.rs

@@ -325,8 +325,8 @@ pub fn super_relate_tys<R: TypeRelation<'tcx>>(
 ) -> RelateResult<'tcx, Ty<'tcx>> {
    let tcx = relation.tcx();
    debug!("super_relate_tys: a={:?} b={:?}", a, b);
-    match (&a.kind(), &b.kind()) {
-        (&ty::Infer(_), _) | (_, &ty::Infer(_)) => {
+    match (a.kind(), b.kind()) {


I hope this was the main reason for the regression. Relate is quite hot and &a.kind() results in suboptimal codegen

compiler/rustc_middle/src/ty/relate.rs

lcnr · 2020-10-03T17:36:55Z

let's try again

@bors try @rust-timer queue

rust-timer · 2020-10-03T17:36:57Z

Awaiting bors try build completion

bors · 2020-10-03T17:37:08Z

⌛ Trying commit 380e230 with merge ab3feb4f1ad1434df143ec7dc8c54097cc1b387c...

bors · 2020-10-03T18:19:33Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: ab3feb4f1ad1434df143ec7dc8c54097cc1b387c (ab3feb4f1ad1434df143ec7dc8c54097cc1b387c)

rust-timer · 2020-10-03T18:19:35Z

Queued ab3feb4f1ad1434df143ec7dc8c54097cc1b387c with parent 738d4a7, future comparison URL.

rust-timer · 2020-10-03T21:32:09Z

Finished benchmarking try commit (ab3feb4f1ad1434df143ec7dc8c54097cc1b387c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

lcnr · 2020-10-06T17:42:40Z

See https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Make.20TyKind.20Copy.20and.20change.20ty.2Ekind%28%29.20to.20.E2.80.A6.20compiler-team.23363/near/212460531 for some discussion about this.

A huge thanks for @LeSeulArtichaut for implementing this MCP!

LeSeulArtichaut added 7 commits October 3, 2020 13:52

Make TyKind Copy and change ty.kind() to return TyKind

bd2bd48

ty.kind() -> TyKind in rustc_{infer,codegen_ssa,codegen_llvm}

4eb1599

ty.kind() -> TyKind in rustc_{lint,metadata,privacy,passes,ty}

7f42113

ty.kind() -> TyKind in rustc_{trait_selection,traits,symbol_mangling}

58e5166

ty.kind() -> TyKind in rustc_typeck

d5c7381

ty.kind() -> TyKind in rustc_{mir,mir_build}

67ba809

ty.kind() -> TyKind in rustdoc and clippy

5268ae7

LeSeulArtichaut added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 3, 2020

rust-highfive assigned lcnr Oct 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 3, 2020

lcnr approved these changes Oct 3, 2020

View reviewed changes

jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. I-compiletime Issue: Problems and improvements with respect to compile times. labels Oct 3, 2020

LeSeulArtichaut force-pushed the copy-tykind branch from f7a8357 to f47fd5d Compare October 3, 2020 15:33

LeSeulArtichaut force-pushed the copy-tykind branch from f47fd5d to 46df4cf Compare October 3, 2020 16:35

lcnr reviewed Oct 3, 2020

View reviewed changes

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

Apply suggestions from code review

380e230

LeSeulArtichaut force-pushed the copy-tykind branch from 46df4cf to 380e230 Compare October 3, 2020 17:23

LeSeulArtichaut closed this Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `TyKind` `Copy` and change `ty.kind()` to return `TyKind` #77482

Make `TyKind` `Copy` and change `ty.kind()` to return `TyKind` #77482

LeSeulArtichaut commented Oct 3, 2020

rust-highfive commented Oct 3, 2020

lcnr commented Oct 3, 2020

bors commented Oct 3, 2020

lcnr left a comment

lcnr Oct 3, 2020

lcnr Oct 3, 2020

lcnr Oct 3, 2020

lcnr commented Oct 3, 2020 •

edited

Loading

rust-timer commented Oct 3, 2020

bors commented Oct 3, 2020

rust-timer commented Oct 3, 2020

rust-timer commented Oct 3, 2020

LeSeulArtichaut commented Oct 3, 2020

lcnr commented Oct 3, 2020

lcnr commented Oct 3, 2020 •

edited

Loading

lcnr Oct 3, 2020 •

edited

Loading

lcnr commented Oct 3, 2020

rust-timer commented Oct 3, 2020

bors commented Oct 3, 2020

bors commented Oct 3, 2020

rust-timer commented Oct 3, 2020

rust-timer commented Oct 3, 2020

lcnr commented Oct 6, 2020

Make TyKind Copy and change ty.kind() to return TyKind #77482

Make TyKind Copy and change ty.kind() to return TyKind #77482

Conversation

LeSeulArtichaut commented Oct 3, 2020

rust-highfive commented Oct 3, 2020

lcnr commented Oct 3, 2020

bors commented Oct 3, 2020

lcnr left a comment

Choose a reason for hiding this comment

lcnr Oct 3, 2020

Choose a reason for hiding this comment

lcnr Oct 3, 2020

Choose a reason for hiding this comment

lcnr Oct 3, 2020

Choose a reason for hiding this comment

lcnr commented Oct 3, 2020 • edited Loading

rust-timer commented Oct 3, 2020

bors commented Oct 3, 2020

rust-timer commented Oct 3, 2020

rust-timer commented Oct 3, 2020

LeSeulArtichaut commented Oct 3, 2020

lcnr commented Oct 3, 2020

lcnr commented Oct 3, 2020 • edited Loading

lcnr Oct 3, 2020 • edited Loading

Choose a reason for hiding this comment

lcnr commented Oct 3, 2020

rust-timer commented Oct 3, 2020

bors commented Oct 3, 2020

bors commented Oct 3, 2020

rust-timer commented Oct 3, 2020

rust-timer commented Oct 3, 2020

lcnr commented Oct 6, 2020

Make `TyKind` `Copy` and change `ty.kind()` to return `TyKind` #77482

Make `TyKind` `Copy` and change `ty.kind()` to return `TyKind` #77482

lcnr commented Oct 3, 2020 •

edited

Loading

lcnr commented Oct 3, 2020 •

edited

Loading

lcnr Oct 3, 2020 •

edited

Loading