Skip to content

Remove HIR inlining (now that we don't need it for constant evaluation anymore) #49690

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.

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

Closed
michaelwoerister opened this issue Apr 5, 2018 · 10 comments
Labels
E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.

Comments

@michaelwoerister
Copy link
Member

Now that we have MIRI-based constant evaluation, we should be able to remove cross-crate HIR inlining and thus a source of mutability in the HIR map. This is needed for query parallelization.

@michaelwoerister michaelwoerister added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. A-parallel-queries labels Apr 5, 2018
@wesleywiser
Copy link
Member

Can somebody point me in the right direction? I tried grepping for HIR inline but didn't find anything conclusive.

@michaelwoerister
Copy link
Member Author

Thanks for taking an interest in this, @wesleywiser!

"HIR inlining" means importing pieces of HIR from external crates into the current HIR map. Historically we have done this for everything that needed to be translated in the current crate (most prominently all generic functions) but with MIR the need to do this mostly went away. Constant evaluation was the last thing we needed this for and with MIRI in place now, we should be able to get rid of it.

The relevant parts are in the HIR map:

pub fn get_inlined_body_untracked(&self, def_id: DefId) -> Option<&'hir Body> {
self.inlined_bodies.borrow().get(&def_id).cloned()
}
pub fn intern_inlined_body(&self, def_id: DefId, body: Body) -> &'hir Body {
let body = self.forest.inlined_bodies.alloc(body);
self.inlined_bodies.borrow_mut().insert(def_id, body);
body
}

and in crate metadata encoding:
https://github.com/rust-lang/rust/blob/8ae79efce3e43eadecd032cf38b2372b4cba7b62/src/librustc_metadata/astencode.rs

Before you go ahead and do anything though, we should confirm with @oli-obk and @eddyb that we can actually remove it.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2018

Rustdoc is still using it in print_inlined_const. Not sure what to do about that. They don't want to print the source, because it might be from macro expansions, and they can't always print the result of a const eval because it might be an associated constant that depends on generic arguments.

@michaelwoerister
Copy link
Member Author

Oh, now I remember you mentioning that, @oli-obk. Not sure how to deal with that. Maybe implement something simpler especially for rustdoc?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2018

We could, specifically for constants, create a "rendered" field that just contains the string that rustdoc would expect anyway

@wesleywiser
Copy link
Member

I found that function yesterday and thought that might be what you were referring to. After removing it, the only tests that failed were a few rustdoc tests around the behavior of inlined external items (as you mentioned). The good news is that the rest of the test suite passed, so it's just rustdoc that needs a fix.

@eddyb
Copy link
Member

eddyb commented Apr 10, 2018

I'd suggest looking at how this is done for function arguments (also only needed by rustdoc):

pub arg_names: LazySeq<ast::Name>,

@michaelwoerister
Copy link
Member Author

Sounds like the way to go. The rendered field could be added to EntryKind::Const (and the static variants, if needed) as a LazySeq<String>.

@wesleywiser
Copy link
Member

Thanks! I'll start working on that.

@michaelwoerister
Copy link
Member Author

Great! Thanks, @wesleywiser!

wesleywiser added a commit to wesleywiser/rust that referenced this issue Apr 16, 2018
bors added a commit that referenced this issue Apr 16, 2018
wesleywiser added a commit to wesleywiser/rust that referenced this issue Apr 20, 2018
bors added a commit that referenced this issue Apr 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.
Projects
None yet
Development

No branches or pull requests

4 participants