Remove HIR inlining#49991

wesleywiser · 2018-04-16T02:25:58Z

Fixes #49690

r? @michaelwoerister

michaelwoerister · 2018-04-16T08:25:05Z

Nice, thanks @wesleywiser! I'll take a look as soon as I find the time.

michaelwoerister · 2018-04-16T14:20:54Z

Looks great, @wesleywiser! Two things I'm wondering:

Does this code also need to handle statics?
Could the schema::Entry::ast field be removed entirely?

oli-obk

👍

oli-obk · 2018-04-16T15:05:00Z

src/librustc_metadata/schema.rs


+#[derive(RustcEncodable, RustcDecodable)]
+pub struct ConstData {
+    pub rendered: String,


Is this expected to gain more fields? If no, how about making t a newtype called RenderedConst?

Also 🚨documentation police🚨 please add some docs

I'm not aware of any additional data we need at this time. I'd be happy to change it and I'll add some docs too 👍

oli-obk · 2018-04-16T15:07:19Z

src/librustc/ty/maps/mod.rs

    [] fn item_attrs: ItemAttrs(DefId) -> Lrc<[ast::Attribute]>,
    [] fn trans_fn_attrs: trans_fn_attrs(DefId) -> TransFnAttrs,
    [] fn fn_arg_names: FnArgNames(DefId) -> Vec<ast::Name>,
+    [] fn rendered_const: RenderedConst(DefId) -> String,


I know this is essentially self explanatory. Still, a short doc comment would be awesome

wesleywiser · 2018-04-16T16:07:09Z

@michaelwoerister

Does this code also need to handle statics?

I don't believe so. I was unable to find a failing test case for statics and all of the existing rustdoc tests pass after handling Const and AssociatedConst.

Could the schema::Entry::ast field be removed entirely?

I think I ran into some issues when I tried that but I don't remember clearly. I will investigate that tonight.

Mark-Simulacrum · 2018-04-16T22:35:23Z

@bors try

bors · 2018-04-16T22:35:34Z

⌛ Trying commit 26b3203 with merge 6bdc8be...

@michaelwoerister

Remove HIR inlining Fixes #49690 r? @michaelwoerister

wesleywiser · 2018-04-16T23:19:13Z

schema::Entry::ast is still used by the typeck_tables_of query which calls item_body_tables and the const_is_rvalue_promotable_to_static query which calls a function of the same name.

bors · 2018-04-17T00:19:17Z

☀️ Test successful - status-travis
State: approved= try=True

wesleywiser · 2018-04-17T00:48:08Z

Added docs & newtype wrapper.

michaelwoerister · 2018-04-17T08:36:35Z

I don't believe so. I was unable to find a failing test case for statics and all of the existing rustdoc tests pass after handling Const and AssociatedConst.

👍

schema::Entry::ast is still used by the typeck_tables_of query which calls item_body_tables and the const_is_rvalue_promotable_to_static query which calls a function of the same name.

You might be able to remove the metadata-based version of the typeck_tables_of query, since it shouldn't be called by for external items anymore. About const_is_rvalue_promotable_to_static I'm not sure. @eddyb or @oli-obk might know more.

bors · 2018-04-17T09:12:21Z

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

oli-obk · 2018-04-17T10:38:37Z

You'd need to move the rvalue_promotable_to_static field up from the Ast to the Entry

eddyb · 2018-04-17T10:52:40Z

src/librustc/middle/cstore.rs

Can this be removed too?

Yes, good catch!

eddyb · 2018-04-17T10:55:06Z

src/librustc_metadata/schema.rs

You could replace the u8 here and in AssociatedConst with a struct ConstQualif { mir: u8, ast_promotable: bool }.

What does ast_promotable mean?

The rvalue_promotable_to_static: bool field that's currently in Ast.

Fixes rust-lang#49690

wesleywiser · 2018-04-20T00:38:33Z

Implemented feedback & rebased.

michaelwoerister · 2018-04-20T08:19:31Z

Thanks a lot, @wesleywiser! Very pleasing to see this go :)

@bors r+

bors · 2018-04-20T08:19:32Z

📌 Commit 4a77d35 has been approved by michaelwoerister

bors · 2018-04-20T08:22:54Z

⌛ Testing commit 4a77d35 with merge 1a44439...

@michaelwoerister

…ister Remove HIR inlining Fixes #49690 r? @michaelwoerister

bors · 2018-04-20T10:40:22Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 1a44439 to master...

varkor · 2018-04-20T11:11:23Z

src/librustc_metadata/encoder.rs

+            } else if let hir::ImplItemKind::Method(ref sig, _) = ast_item.node {
+                let generics = self.tcx.generics_of(def_id);
+                let types = generics.parent_types as usize + generics.types.len();
+                let needs_inline = types > 0 || tcx.trans_fn_attrs(def_id).requests_inline() &&


This changes the behaviour as && binds more tightly — was this intentional?

No, that was not intentional. Good catch!

Also, this would be nicer with a match instead of chained if lets. @wesleywiser, would you mind making a PR with a fix?

Yeah, looks like I messed this up when I pulled the latest changes into my branch. I've opened a quick PR that fixes this (#50114). I'll send a follow up tonight that uses a match for this instead.

When I rebased rust-lang#49991 on `master`, I messed up the merge for this line. I'm reverting this back to the way it was in f15e5c1.

@michaelwoerister

…ster Fix bad merge in rust-lang#49991 When I rebased rust-lang#49991 on `master`, I messed up the merge for this line. I'm reverting this back to the way it was in f15e5c1. r? @michaelwoerister

Rollup of 7 pull requests Successful merges: - #50031 (Clarified E0015 message.) - #50058 (Added build disk usage information) - #50081 (Update stdsimd submodule) - #50083 (wasm: Increase default stack size to 1MB) - #50104 (Disable auto-detection of libxml2 when compiling llvm.) - #50114 (Fix bad merge in #49991) - #50117 (must explicitly request file name when using with_file_name.) Failed merges:

Clean up `IsolatedEncoder::encode_info_for_impl_item()` a bit Suggested in the [comments of #49991](https://github.com/rust-lang/rust/pull/49991/files/4a77d35c1ed89310a0ed128ce931cd4b85ca4cd4#r183048939)

eddyb · 2018-04-24T18:38:10Z

src/librustc_metadata/schema.rs

                associated_container.hash_stable(hcx, hasher);
            }
-            EntryKind::AssociatedConst(associated_container, qualif) => {
+            EntryKind::AssociatedConst(associated_container, qualif, _) => {


This ignores the last field, but EntryKind::Const above doesn't. cc @michaelwoerister

Yeah, that's wrong. I'll open a PR for that.

eddyb · 2018-04-24T18:39:23Z

src/librustc_metadata/encoder.rs

        !self.tcx.sess.opts.output_types.should_trans()
    }

+    fn const_qualif(&self, mir: u8, body_id: hir::BodyId) -> ConstQualif {


This should ideally call mir_const_qualif itself instead of taking the value as an argument.

I didn't do that because of this call https://github.com/rust-lang/rust/pull/49991/files/4a77d35c1ed89310a0ed128ce931cd4b85ca4cd4#diff-f55f5d2e86bc0e08c341b51fc78b27e4R796

Why would that keep the AST promotability but not the MIR one? I think you just never need either for trait associated consts.

Related to rust-lang#49991

@michaelwoerister

Hash EntryKind::AssociatedConst const data Related to rust-lang#49991 r? @michaelwoerister cc @eddyb

Related to rust-lang#49991

rust-highfive assigned michaelwoerister Apr 16, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 16, 2018

oli-obk reviewed Apr 16, 2018

View reviewed changes

bors added a commit that referenced this pull request Apr 16, 2018

Auto merge of #49991 - wesleywiser:remove_hir_inlining, r=<try>

6bdc8be

Remove HIR inlining Fixes #49690 r? @michaelwoerister

wesleywiser force-pushed the remove_hir_inlining branch from 26b3203 to fee5c82 Compare April 17, 2018 00:47

eddyb reviewed Apr 17, 2018

View reviewed changes

Remove HIR inlining

4a77d35

Fixes rust-lang#49690

wesleywiser force-pushed the remove_hir_inlining branch from fee5c82 to 4a77d35 Compare April 20, 2018 00:34

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 Apr 20, 2018

bors added a commit that referenced this pull request Apr 20, 2018

Auto merge of #49991 - wesleywiser:remove_hir_inlining, r=michaelwoer…

1a44439

…ister Remove HIR inlining Fixes #49690 r? @michaelwoerister

bors merged commit 4a77d35 into rust-lang:master Apr 20, 2018

bors mentioned this pull request Apr 20, 2018

The Great Generics Generalisation: Ty Edition #48523

Merged

varkor reviewed Apr 20, 2018

View reviewed changes

wesleywiser added a commit to wesleywiser/rust that referenced this pull request Apr 20, 2018

Fix bad merge in rust-lang#49991

ee9a472

When I rebased rust-lang#49991 on `master`, I messed up the merge for this line. I'm reverting this back to the way it was in f15e5c1.

wesleywiser mentioned this pull request Apr 20, 2018

Fix bad merge in #49991 #50114

Merged

kennytm mentioned this pull request Apr 20, 2018

Rollup of 7 pull requests #50119

Merged

eddyb reviewed Apr 24, 2018

View reviewed changes

wesleywiser added a commit to wesleywiser/rust that referenced this pull request Apr 25, 2018

Hash EntryKind::AssociatedConst const data

d81d081

Related to rust-lang#49991

wesleywiser mentioned this pull request Apr 25, 2018

Hash EntryKind::AssociatedConst const data #50207

Merged

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 26, 2018

Rollup merge of rust-lang#50207 - wesleywiser:hash_const_data, r=eddyb

0d758ec

Hash EntryKind::AssociatedConst const data Related to rust-lang#49991 r? @michaelwoerister cc @eddyb

U007D pushed a commit to humanenginuity/rust that referenced this pull request Apr 30, 2018

Hash EntryKind::AssociatedConst const data

19aa82a

Related to rust-lang#49991

Uh oh!

Conversation

wesleywiser commented Apr 16, 2018

Uh oh!

michaelwoerister commented Apr 16, 2018

Uh oh!

michaelwoerister commented Apr 16, 2018

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesleywiser commented Apr 16, 2018

Uh oh!

Mark-Simulacrum commented Apr 16, 2018

Uh oh!

bors commented Apr 16, 2018

Uh oh!

wesleywiser commented Apr 16, 2018

Uh oh!

bors commented Apr 17, 2018

Uh oh!

wesleywiser commented Apr 17, 2018

Uh oh!

michaelwoerister commented Apr 17, 2018

Uh oh!

bors commented Apr 17, 2018

Uh oh!

oli-obk commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesleywiser commented Apr 20, 2018

Uh oh!

michaelwoerister commented Apr 20, 2018

Uh oh!

bors commented Apr 20, 2018

Uh oh!

bors commented Apr 20, 2018

Uh oh!

bors commented Apr 20, 2018

Uh oh!

varkor Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

oli-obk commented Apr 17, 2018 •

edited

Loading

varkor Apr 20, 2018 •

edited

Loading