Skip to content

Refactor vtable codegen - #86291

Merged
bors merged 1 commit into
rust-lang:masterfrom
crlf0710:trait_vtbl_refactor
Jun 16, 2021
Merged

Refactor vtable codegen#86291
bors merged 1 commit into
rust-lang:masterfrom
crlf0710:trait_vtbl_refactor

Conversation

@crlf0710

@crlf0710crlf0710 commented Jun 14, 2021•
edited
Loading

Copy link
Copy Markdown
Member

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

rust-highfive commented Jun 14, 2021

Copy link
Copy Markdown
Contributor

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive

rust-highfive commented Jun 14, 2021

Copy link
Copy Markdown
Contributor

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-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
leonardo-m
leonardo-m reviewed Jun 14, 2021
View reviewed changes
Comment threadcompiler/rustc_codegen_cranelift/src/vtable.rs Outdated
@crlf0710
crlf0710force-pushed the trait_vtbl_refactor branch from 7fd6b81 to d8937cbCompareJune 14, 2021 11:08
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710
crlf0710force-pushed the trait_vtbl_refactor branch from d8937cb to 80a6cefCompareJune 14, 2021 11:14
@bjorn3

bjorn3 commented Jun 14, 2021

Copy link
Copy Markdown
Member

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}?

bjorn3
bjorn3 reviewed Jun 14, 2021
View reviewed changes
Comment threadcompiler/rustc_middle/src/ty/mod.rs Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710
crlf0710force-pushed the trait_vtbl_refactor branch from 80a6cef to 7b237e7CompareJune 14, 2021 11:31
@petrochenkov

petrochenkov commented Jun 14, 2021

Copy link
Copy Markdown
Contributor

r? @bjorn3

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3

bjorn3 commented Jun 14, 2021

Copy link
Copy Markdown
Member

That seems like it is a miscompilation.

@crlf0710

crlf0710 commented Jun 14, 2021

Copy link
Copy Markdown
MemberAuthor

Indeed, i'll try to investigate.

RalfJung
RalfJung reviewed Jun 14, 2021
View reviewed changes
Comment threadcompiler/rustc_mir/src/interpret/traits.rs Outdated

@RalfJungRalfJungJun 14, 2021

Copy link
Copy Markdown
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. :)

RalfJung
RalfJung reviewed Jun 14, 2021
View reviewed changes
Comment threadcompiler/rustc_mir/src/interpret/traits.rs Outdated
@crlf0710
crlf0710force-pushed the trait_vtbl_refactor branch from 7b237e7 to 7be091eCompareJune 14, 2021 16:00
@crlf0710

crlf0710 commented Jun 14, 2021

Copy link
Copy Markdown
MemberAuthor

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 :(

@crlf0710crlf0710 added the F-trait_upcasting `#![feature(trait_upcasting)]` label Jun 14, 2021
RalfJung
RalfJung reviewed Jun 14, 2021
View reviewed changes
Comment threadcompiler/rustc_mir/src/interpret/traits.rs Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710
crlf0710force-pushed the trait_vtbl_refactor branch from 7be091e to a86d3a7CompareJune 14, 2021 18:01
@crlf0710crlf0710 mentioned this pull request Jun 14, 2021
Closed
21 tasks
@crlf0710

crlf0710 commented Jun 14, 2021

Copy link
Copy Markdown
MemberAuthor

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 bjorn3June 15, 2021 00:19
@crlf0710

crlf0710 commented Jun 15, 2021•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

RalfJung commented Jun 15, 2021

Copy link
Copy Markdown
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

bjorn3 commented Jun 15, 2021

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 15, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit a86d3a7 has been approved by bjorn3

@borsbors 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

bjorn3 commented Jun 15, 2021

Copy link
Copy Markdown
Member

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.

@crlf0710crlf0710 mentioned this pull request Jun 15, 2021
Closed
@crlf0710

crlf0710 commented Jun 15, 2021

Copy link
Copy Markdown
MemberAuthor

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

@bors

bors commented Jun 16, 2021

Copy link
Copy Markdown
Collaborator

⌛ Testing commit a86d3a7 with merge 2336406...

@bors

bors commented Jun 16, 2021

Copy link
Copy Markdown
Collaborator

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

@borsbors added the merged-by-bors This PR was explicitly merged by bors. label Jun 16, 2021
@bors
bors merged commit 2336406 into rust-lang:masterJun 16, 2021
@rustbotrustbot added this to the 1.55.0 milestone Jun 16, 2021
@crlf0710
crlf0710 deleted the trait_vtbl_refactor branch June 16, 2021 10:19
@borsbors mentioned this pull request Jun 16, 2021
Merged
bjorn3
bjorn3 reviewed Jun 17, 2021
View reviewed changes
Comment threadcompiler/rustc_codegen_cranelift/src/vtable.rs
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jul 7, 2021
@bors
Auto merge ofrust-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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJungRalfJungRalfJung left review comments
@bjorn3bjorn3bjorn3 left review comments
+1 more reviewer
@leonardo-mleonardo-mleonardo-m left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@bjorn3bjorn3

Labels

F-trait_upcasting`#![feature(trait_upcasting)]`merged-by-borsThis PR was explicitly merged by bors.S-waiting-on-borsStatus: 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