Skip to content

[mir-inlining] Don't inline virtual calls #55046

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 15, 2018
Merged

[mir-inlining] Don't inline virtual calls #55046

merged 1 commit into from
Oct 15, 2018

Conversation

wesleywiser
Copy link
Member

No description provided.

@rust-highfive
Copy link
Contributor

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2018
varkor
varkor reviewed Oct 14, 2018
View reviewed changes
src/librustc_mir/transform/inline.rs Outdated
let is_virtual =
if let InstanceDef::Virtual(..) = instance.def {
true
} else { false };
Copy link
Member

Choose a reason for hiding this comment

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

Nit: false should be on its own line.

varkor
varkor reviewed Oct 14, 2018
View reviewed changes
src/test/mir-opt/inline-trait-method.rs Outdated
@@ -0,0 +1,41 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
Copy link
Member

Choose a reason for hiding this comment

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

Copyright is no longer needed.

varkor
varkor reviewed Oct 14, 2018
View reviewed changes
src/test/mir-opt/inline-trait-method.rs Outdated
println!("{}", test(&()));
}

fn test(x: &X) -> u32 {
Copy link
Member

@varkor varkor Oct 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Prefer &dyn X over &X.

@varkor
Copy link
Member

varkor commented Oct 14, 2018

Was it inlining the default function implementation, or the implementation for () previously? Either way, r=me after the comments are addressed.

@wesleywiser
Copy link
Member Author

It was inlining the default implementation so prior to the patch, the test would output 1 instead of 2 like it should.

@wesleywiser wesleywiser force-pushed the no_virtual_call_inlining branch from bc47afd to 6c2dace Compare October 14, 2018 17:43
@wesleywiser
Copy link
Member Author

Feedback resolved. Thanks @varkor!

@wesleywiser
[mir-inlining] Don't inline virtual calls
69eaa11 
Prior to this change, the test case would output `1` instead of `2` like
it should.
@wesleywiser wesleywiser force-pushed the no_virtual_call_inlining branch from 6c2dace to 69eaa11 Compare October 14, 2018 17:46
@wesleywiser
Copy link
Member Author

@bors r=varkor

@bors
Copy link
Collaborator

bors commented Oct 14, 2018

📌 Commit 69eaa11 has been approved by varkor

@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 Oct 14, 2018
@bors
Copy link
Collaborator

bors commented Oct 14, 2018

⌛ Testing commit 69eaa11 with merge 0c665e2...

bors added a commit that referenced this pull request Oct 14, 2018
@bors
Auto merge of #55046 - wesleywiser:no_virtual_call_inlining, r=varkor
0c665e2 
[mir-inlining] Don't inline virtual calls
@bors
Copy link
Collaborator

bors commented Oct 15, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: varkor
Pushing 0c665e2 to master...

@bors bors merged commit 69eaa11 into rust-lang:master Oct 15, 2018
@RalfJung RalfJung mentioned this pull request Oct 15, 2018
Closed
@RalfJung
Copy link
Member

RalfJung commented Oct 15, 2018 •
edited
Loading

I think this fixed #50411, and running the miri test suite with MIR optimizations. :)

Would be great if the reproducer from #50411 could be added as a test case.

EDIT: Ah nvm, I'll just quickly do that. So happy that this works, I don't want to see it regressing again :)

@RalfJung RalfJung mentioned this pull request Oct 15, 2018
Closed
@RalfJung
Copy link
Member

Just, something strange is going on, and while the tests pass locally they fail on CI?!? See #55086.

@wesleywiser
Copy link
Member Author

@RalfJung Is there anything that needs to be done related to this bug fix? I see you closed the other pull request.

@RalfJung
Copy link
Member

Yeah, turns out I was wrong when I thought that the ICE got fixed. :( Sorry for the noise.

@wesleywiser
Copy link
Member Author

No worries :)

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

@varkor varkor varkor left review comments

Assignees

@varkor varkor

Labels
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.
5 participants
@wesleywiser @rust-highfive @varkor @bors @RalfJung