Skip to content

Create rustc_type_ir#79169

Merged
bors merged 1 commit intorust-lang:masterfrom
LeSeulArtichaut:ty-lib
Dec 12, 2020
Merged

Create rustc_type_ir#79169
bors merged 1 commit intorust-lang:masterfrom
LeSeulArtichaut:ty-lib

Conversation

@LeSeulArtichaut
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut commented Nov 18, 2020 •
edited
Loading

Decided to start small 😄

This PR creates a rustc_type_ir crate as part of the WG-Traits plan to create a shared type library.
There already exists a rustc_ty crate, so I named the new crate rustc_ty_library. However I think it would make sense to rename the current rustc_ty to something else (e.g. rustc_ty_passes) to free the name for this new crate.

r? @jackh726

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2020
@jackh726
Copy link
Member

jackh726 commented Nov 18, 2020

I haven't read through this yet, but I agree that we should rename rustc_ty to rustc_ty_passes (or similiar), and probably prior to this. Maybe make a separate PR. This is probably MCP territory, but I think the rename would probably be fine. Though we'll need @rust-lang/compiler signoff.

@LeSeulArtichaut LeSeulArtichaut force-pushed the ty-lib branch 3 times, most recently from 8df14f2 to 2df3ece Compare November 18, 2020 18:26
jackh726
jackh726 approved these changes Nov 18, 2020
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

This LGTM! I'm on board with these changes as-is (only a fairly small subset of what will eventually get moved out), but I guess there are couple questions/concerns others might have:

  1. Should we be moving out more? I think no since we've discussed this in wg-traits meetings and decided any start is betting than nothing.
  2. Should things still have a shorthand, like ty::INNERMOST to tylib::INNERMOST or something like that?
  3. Should this wait until after rust_ty MCP? Should this have an MCP itself? I think it's best to probably discuss this on Zulip.

And finally, let's r? @nikomatsakis, since I don't have r+ rights.

@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented Nov 18, 2020

r? @nikomatsakis

@jackh726
Copy link
Member

jackh726 commented Nov 18, 2020

Should things still have a shorthand, like ty::INNERMOST to tylib::INNERMOST or something like that?

Another alternative would be to just make rustc_middle reexport rustc_ty_library items.

@jyn514
Copy link
Member

jyn514 commented Nov 18, 2020

library is not really descriptive of what this does. Could you call it rustc_ty_data instead?

@jyn514 jyn514 added A-synthetic-impls Area: Synthetic impls, used by rustdoc to document auto traits and traits with blanket impls A-type-system Area: Type system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed A-synthetic-impls Area: Synthetic impls, used by rustdoc to document auto traits and traits with blanket impls labels Nov 18, 2020
cjgillot
cjgillot reviewed Nov 18, 2020
View reviewed changes
jackh726
jackh726 approved these changes Dec 2, 2020
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

This is a good start. Simple, but good.

@LeSeulArtichaut LeSeulArtichaut changed the title Create rustc_ty_library Create rustc_type_ir Dec 3, 2020
@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Dec 12, 2020
Merged
nikomatsakis
nikomatsakis approved these changes Dec 12, 2020
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Dec 12, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 12, 2020

📌 Commit 0cf5a8a has been approved by nikomatsakis

@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 Dec 12, 2020
lqd
lqd reviewed Dec 12, 2020
View reviewed changes
@bors
Copy link
Collaborator

bors commented Dec 12, 2020

⌛ Testing commit 0cf5a8a with merge 3f2088a...

@bors
Copy link
Collaborator

bors commented Dec 12, 2020

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 3f2088a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 12, 2020
@bors bors merged commit 3f2088a into rust-lang:master Dec 12, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 12, 2020
@LeSeulArtichaut LeSeulArtichaut deleted the ty-lib branch December 12, 2020 15:03
@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Dec 12, 2020
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 12, 2020
@Dylan-DPC
Rollup merge of rust-lang#79963 - LeSeulArtichaut:debruijn-typo, r=Dy…
e3af345 
…lan-DPC

Fix typo in `DebruijnIndex` documentation

Suggested in rust-lang#79169 (comment).
r? `@lqd`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 13, 2020
@JohnTitor
Rollup merge of rust-lang#79963 - LeSeulArtichaut:debruijn-typo, r=Dy…
e4a663c 
…lan-DPC

Fix typo in `DebruijnIndex` documentation

Suggested in rust-lang#79169 (comment).
r? ``@lqd``
@rylev
Copy link
Member

rylev commented Dec 15, 2020

@LeSeulArtichaut @nikomatsakis this PR had some performance regressions in the const evaluation stress test (but not in other benchmarks). Is this change simply moving code around? I wonder if something that was previously being inlined is now not.

@jyn514
Copy link
Member

jyn514 commented Dec 15, 2020

Self-profile shows almost all the time being spent in eval_to_allocation_raw:

Screenshot_20201215-111057_Ecosia

@jackh726
Copy link
Member

jackh726 commented Dec 15, 2020

@LeSeulArtichaut @nikomatsakis this PR had some performance regressions in the const evaluation stress test (but not in other benchmarks). Is this change simply moving code around? I wonder if something that was previously being inlined is now not.

Yes, this is literally just moving code around. Didn't expect a perf regression at all. I do imagine something isn't getting inlined anymore.

@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Dec 15, 2020
Closed
@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented Dec 15, 2020

Filed #80057 to add #[inline] to the DebruijnIndex methods

@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Dec 17, 2020
Closed
@mati865
Copy link
Member

mati865 commented Dec 18, 2020

Yes, this is literally just moving code around. Didn't expect a perf regression at all.

It's fairly common that PRs moving the code to a new crate regress performance a little.

I do imagine something isn't getting inlined anymore.

That's often the reason.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 28, 2021
@JohnTitor
Rollup merge of rust-lang#79951 - LeSeulArtichaut:ty-ir, r=nikomatsakis
446edd1 
Refractor a few more types to `rustc_type_ir`

In the continuation of rust-lang#79169, ~~blocked on that PR~~.

This PR:
 - moves `IntVarValue`, `FloatVarValue`, `InferTy` (and friends) and `Variance`
 - creates the `IntTy`, `UintTy` and `FloatTy` enums in `rustc_type_ir`, based on their `ast` and `chalk_ir` equilavents, and uses them for types in the rest of the compiler.

~~I will split up that commit to make this easier to review and to have a better commit history.~~
EDIT: done, I split the PR in commits of 200-ish lines each

r? `````@nikomatsakis````` cc `````@jackh726`````
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 30, 2021
@JohnTitor
Rollup merge of rust-lang#79951 - LeSeulArtichaut:ty-ir, r=nikomatsakis
7bd3d6c 
Refractor a few more types to `rustc_type_ir`

In the continuation of rust-lang#79169, ~~blocked on that PR~~.

This PR:
 - moves `IntVarValue`, `FloatVarValue`, `InferTy` (and friends) and `Variance`
 - creates the `IntTy`, `UintTy` and `FloatTy` enums in `rustc_type_ir`, based on their `ast` and `chalk_ir` equilavents, and uses them for types in the rest of the compiler.

~~I will split up that commit to make this easier to review and to have a better commit history.~~
EDIT: done, I split the PR in commits of 200-ish lines each

r? `````@nikomatsakis````` cc `````@jackh726`````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lqd lqd lqd left review comments

@cjgillot cjgillot cjgillot left review comments

@nikomatsakis nikomatsakis nikomatsakis approved these changes

@jackh726 jackh726 jackh726 approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels

A-type-system Area: Type system 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

Milestone

1.50.0

Development

Successfully merging this pull request may close these issues.