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.

Sign up for GitHub

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

Jump to bottom

Upgrade Chalk #72936

Merged
merged 15 commits into from
Jun 21, 2020
Merged

Upgrade Chalk #72936

merged 15 commits into from
Jun 21, 2020

Conversation

jackh726
Copy link
Member

@jackh726 jackh726 commented Jun 3, 2020

Things done in this PR:

  • Upgrade Chalk to 0.11.0
  • Added compare-mode=chalk
  • Bump rustc-hash in librustc_data_structures to 1.1.0 to match Chalk
  • Removed RustDefId since the builtin type support is there
  • Add a few more FIXME(chalk)s for problem spots I hit when running all tests with chalk
  • Added some more implementation code for some newer builtin Chalk types (e.g. FnDef, Array)
  • Lower RegionOutlives and ObjectSafe predicates
  • Lower Dyn without the region
  • Handle Int/Float CanonicalVarKinds
  • Uncomment some Chalk tests that actually work now
  • Remove the revisions in src/test/ui/coherence/coherence-subtyping.rs since they aren't doing anything different

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 3, 2020
nikomatsakis
nikomatsakis requested changes Jun 3, 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.

Looking good! Exciting to see the progress here. Left some nits.

src/librustc_traits/chalk/db.rs Outdated Show resolved Hide resolved
src/librustc_traits/chalk/mod.rs Outdated Show resolved Hide resolved
src/librustc_middle/traits/chalk.rs Outdated Show resolved Hide resolved
src/librustc_traits/chalk/db.rs Outdated Show resolved Hide resolved
src/librustc_traits/chalk/db.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jun 3, 2020

☔ The latest upstream changes (presumably #70107) made this pull request unmergeable. Please resolve the merge conflicts.

@jackh726
Copy link
Member Author

jackh726 commented Jun 3, 2020 •
edited
Loading

@nikomatsakis addressed your comments

  • Removed the commented out assert
  • Changed skip_binders to no_bound_vars().expect(). This requires collecting into an intermediate Vec, since inputs() returns a slice, which doesn't impl TypeFoldable. But since this is going to be fixed on the Chalk side anyways, I don't think this is a problem right now. It did catch the fact that we were ignoring this in the inherent_impl test, so I commented out the bits of code that fail now :(
  • Can't change InternedAdtDef to AdtDef since it has a Copy bound and AdtDef isn't.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2020

📌 Commit fc8e10d5965c1d4954706cc080ba3a1b0bf06879 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 Jun 4, 2020
@bors
Copy link
Contributor

bors commented Jun 6, 2020

☔ The latest upstream changes (presumably #72927) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 6, 2020
@jackh726
Copy link
Member Author

jackh726 commented Jun 7, 2020

I rebased. Can I get this r+ed again? @nikomatsakis

@Aaron1011
Copy link
Member

@jackh726: It looks like you've modified the llvm-project submodule: was that intentional?

@jackh726 jackh726 force-pushed the chalk-more branch 3 times, most recently from 6f44abe to 4cf2833 Compare June 8, 2020 03:25
@jackh726
Copy link
Member Author

jackh726 commented Jun 8, 2020

@Aaron1011 😔 no that happened during rebase. Fixed.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 8, 2020

📌 Commit 4cf2833 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2020
@Dylan-DPC-zz
Copy link

@bors p=1

@bors
Copy link
Contributor

bors commented Jun 12, 2020

⌛ Testing commit 4cf2833 with merge d73e24b4f387054673f9ca9fa3a9deb3d6b73ba4...

@bors
Copy link
Contributor

bors commented Jun 12, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 12, 2020
@Aaron1011
Copy link
Member

My minimized example from #73335 no longer causes Rustdoc to OOM after #73452 was merged. This PR should now be unblocked.

@Aaron1011 Aaron1011 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jun 20, 2020
@jackh726
Copy link
Member Author

@nikomatsakis care to r+ again?

@Dylan-DPC-zz
Copy link

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 20, 2020

📌 Commit d63195b 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 Jun 20, 2020
@bors
Copy link
Contributor

bors commented Jun 21, 2020

⌛ Testing commit d63195b with merge a8cf399...

@bors
Copy link
Contributor

bors commented Jun 21, 2020

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing a8cf399 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2020
@bors bors merged commit a8cf399 into rust-lang:master Jun 21, 2020
@bors bors mentioned this pull request Jun 21, 2020
Merged
@jackh726 jackh726 deleted the chalk-more branch June 21, 2020 22:28
@nnethercote
Copy link
Contributor

This was a moderate perf win. Nice job!

@jackh726
Copy link
Member Author

@nnethercote that's really unexpected, since basically all of this code doesn't run unless you're in -Z chalk. The only thing I can imagine giving a perf win would be the rustc_hash bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis requested changes

Assignees

@nikomatsakis nikomatsakis

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@jackh726 @bors @nikomatsakis @Aaron1011 @Dylan-DPC-zz @RalfJung @matthewjasper @nnethercote @rust-highfive @rustbot