-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Don't inline virtual calls (take 2) #55802
Don't inline virtual calls (take 2) #55802
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
Thanks for the PR, @wesleywiser! @rust-lang/compiler, who is best suited for reviewing MIR inlining changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nits
src/librustc_mir/transform/inline.rs
Outdated
substs) { | ||
let is_virtual = | ||
if let InstanceDef::Virtual(..) = instance.def { | ||
true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is now its own function, you can get rid of is_virtual
and just return None
here.
src/librustc_mir/transform/inline.rs
Outdated
let terminator = bb_data.terminator(); | ||
if let TerminatorKind::Call { func: ref op, .. } = terminator.kind { | ||
if let ty::FnDef(callee_def_id, substs) = op.ty(caller_mir, self.tcx).sty { | ||
if let Some(instance) = Instance::resolve(self.tcx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can now just be let instance = Instance::resolve(....)?;
☔ The latest upstream changes (presumably #55803) made this pull request unmergeable. Please resolve the merge conflicts. |
When I fixed the previous mis-optimizations, I didn't realize there were actually two different places where we mutate `callsites` and both of them should have the same behavior. As a result, if a function was inlined and that function contained virtual function calls, they were incorrectly being inlined. I also added a test case which covers this.
f80b1a3
to
3cce5c7
Compare
Good ideas @oli-obk! |
@bors r+ |
📌 Commit 3cce5c7 has been approved by |
…c_boogaloo, r=nagisa Don't inline virtual calls (take 2) When I fixed the previous mis-optimizations, I didn't realize there were actually two different places where we mutate `callsites` and both of them should have the same behavior. As a result, if a function was inlined and that function contained virtual function calls, they were incorrectly being inlined. I also added a test case which covers this.
Rollup of 17 pull requests Successful merges: - #55630 (resolve: Filter away macro prelude in modules with `#[no_implicit_prelude]` on 2018 edition) - #55687 (Take supertraits into account when calculating associated types) - #55745 (Convert `outlives_components`' return value to a `SmallVec` outparam.) - #55764 (Fix Rc/Arc allocation layout) - #55792 (Prevent ICE in const-prop array oob check) - #55799 (Removed unneeded instance of `// revisions` from a lint test) - #55800 (Fix ICE in `return_type_impl_trait`) - #55801 (NLL: Update box insensitivity test) - #55802 (Don't inline virtual calls (take 2)) - #55816 (Use `SmallVec` to avoid allocations in `from_decimal_string`.) - #55819 (Typecheck patterns of all match arms first, so we get types for bindings) - #55822 (ICE with #![feature(nll)] and elided lifetimes) - #55828 (Add missing `rustc_promotable` attribute to unsigned `min_value` and `max_value`) - #55839 (Fix docstring spelling mistakes) - #55844 (Fix documentation typos.) - #55845 (Set BINARYEN_TRAP_MODE=clamp) - #55856 (rustdoc: refactor: move all static-file include!s into a single module)
When I fixed the previous mis-optimizations, I didn't realize there were
actually two different places where we mutate
callsites
and both ofthem should have the same behavior.
As a result, if a function was inlined and that function contained
virtual function calls, they were incorrectly being inlined. I also
added a test case which covers this.