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

Refactor vtable codegen #86291

Merged
merged 1 commit into from
Jun 16, 2021
Merged

Refactor vtable codegen #86291

merged 1 commit into from
Jun 16, 2021

Conversation

crlf0710
Copy link
Member

@crlf0710 crlf0710 commented Jun 14, 2021 •
edited
Loading

This refactor the codegen of vtables of miri interpreter, llvm, cranelift codegen backends.

This is preparation for the implementation of trait upcasting feature. cc #65991

Note that aside from code reorganization, there's an internal behavior change here that now InstanceDef::Virtual's index now include the three metadata slots, and now the first method is with index 3.

cc @RalfJung @bjorn3

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Jun 14, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member

bjorn3 commented Jun 14, 2021

Would it be possible to have miri create an Allocation for the vtable and then handle it like a normal constant in cg_{clif,llvm}?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

r? @bjorn3

@rust-highfive rust-highfive assigned bjorn3 and unassigned petrochenkov Jun 14, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member

bjorn3 commented Jun 14, 2021

That seems like it is a miscompilation.

@crlf0710
Copy link
Member Author

Indeed, i'll try to investigate.

RalfJung
RalfJung reviewed Jun 14, 2021
View reviewed changes
compiler/rustc_mir/src/interpret/traits.rs
}
}
})
.collect::<Result<Vec<_>, _>>()?;
Copy link
Member

Choose a reason for hiding this comment

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

This is very elegant. :)

@crlf0710
Copy link
Member Author

Seems i found the bug. I need to add an initial 3 in function vtable_trait_first_method_offset according to the new indexing scheme. There's also a 3 that should be removed in miri part... Magic numbers are so easy to get wrong :(

@crlf0710 crlf0710 added the F-trait_upcasting `#![feature(trait_upcasting)]` label Jun 14, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 mentioned this pull request Jun 14, 2021
Open
19 tasks
@crlf0710
Copy link
Member Author

I think i've found the bug and got it fixed... let's wait to see if the test suite has anything more to say here.

@crlf0710 crlf0710 requested a review from bjorn3 June 15, 2021 00:19
@crlf0710
Copy link
Member Author

crlf0710 commented Jun 15, 2021 •
edited
Loading

This is ready for review again. @bjorn3

Would it be possible to have miri create an Allocation for the vtable and then handle it like a normal constant in cg_{clif,llvm}?

I'll leave this question to @RalfJung , if needed i can create another pr for this, but i guess i'll need some mentoring.

@RalfJung
Copy link
Member

I think that would be nice, but it seems orthogonal to this PR so should probably be done separately.

I know very little about the codegen backends so I don't think I can do much mentoring for this refactor...

@bjorn3
Copy link
Member

bjorn3 commented Jun 15, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jun 15, 2021

📌 Commit a86d3a7 has been approved by bjorn3

@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 15, 2021
@bjorn3
Copy link
Member

bjorn3 commented Jun 15, 2021

I'll leave this question to @RalfJung , if needed i can create another pr for this, but i guess i'll need some mentoring.

The first step would be to refactor the vtable generation in miri to create an Allocation outside of the context of a Machine. In the place of the vtable codegen in cg_{clif,ssa} this function could be called followed by a call to whatever method is used to lower Allocations to backend constants. It may also be nice to add a map from trait + type -> allocation to tcx.alloc_map or something like that to replace the interning done by the backends.

@crlf0710 crlf0710 mentioned this pull request Jun 15, 2021
Closed
@crlf0710
Copy link
Member Author

Thanks! I've recorded this in #86324 to avoid forgetting about it.

@bors
Copy link
Contributor

bors commented Jun 16, 2021

⌛ Testing commit a86d3a7 with merge 2336406...

@bors
Copy link
Contributor

bors commented Jun 16, 2021

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing 2336406 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 16, 2021
@bors bors merged commit 2336406 into rust-lang:master Jun 16, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 16, 2021
@crlf0710 crlf0710 deleted the trait_vtbl_refactor branch June 16, 2021 10:19
@bors bors mentioned this pull request Jun 16, 2021
Merged
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jul 7, 2021
@bors
Auto merge of rust-lang#86291 - crlf0710:trait_vtbl_refactor, r=bjorn3
9be7ca0 
Refactor vtable codegen

This refactor the codegen of vtables of miri interpreter, llvm, cranelift codegen backends.

This is preparation for the implementation of trait upcasting feature. cc rust-lang#65991

Note that aside from code reorganization, there's an internal behavior change here that now InstanceDef::Virtual's index now include the three metadata slots, and now the first method is with index 3.

cc  `@RalfJung` `@bjorn3`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@leonardo-m leonardo-m leonardo-m left review comments

Assignees

@bjorn3 bjorn3

Labels
F-trait_upcasting `#![feature(trait_upcasting)]` 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
1.55.0
Development

Successfully merging this pull request may close these issues.
9 participants
@crlf0710 @rust-highfive @rust-log-analyzer @bjorn3 @petrochenkov @RalfJung @bors @leonardo-m @rustbot