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

Add primary marker on codegen unit and generate main wrapper on primary codegen. #84507

Merged
merged 1 commit into from
May 10, 2021
Merged

Add primary marker on codegen unit and generate main wrapper on primary codegen. #84507

merged 1 commit into from
May 10, 2021

Conversation

crlf0710
Copy link
Member

@crlf0710 crlf0710 commented Apr 24, 2021 •
edited

This is the codegen part of changes extracted from #84062.

This add a marker called primary on each codegen units, where exactly one codegen unit will be primary = true at a time. This specific codegen unit will take charge of generating main wrapper when main is imported from a foreign crate after the implementation of RFC 1260.

cc #28937

I'm not sure who should i ask for review for codegen changes, so feel free to reassign.
r? @nagisa

@crlf0710 crlf0710 added the S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. label Apr 24, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from 37490ab to c5406cd Compare April 24, 2021 09:07
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from c5406cd to 7167212 Compare April 24, 2021 09:28
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from 4fc092b to 977c2a7 Compare April 24, 2021 10:18
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from 44150ec to 6b9e621 Compare April 24, 2021 11:53
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from 6b9e621 to dea2655 Compare April 24, 2021 13:15
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from dea2655 to 6c7abc8 Compare April 24, 2021 13:33
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from 6c7abc8 to 268479e Compare April 24, 2021 14:20
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 25, 2021

☔ The latest upstream changes (presumably #84310) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 25, 2021
@crlf0710 crlf0710 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 25, 2021
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch 3 times, most recently from 9182f04 to 231b44c Compare April 30, 2021 13:52
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Apr 30, 2021
@crlf0710 crlf0710 changed the title [EXPERIMENTAL] Add primary marker on codegen unit. Add primary marker on codegen unit and generate main wrapper on primary codegen. Apr 30, 2021
@crlf0710
Copy link
Member Author

cc @bjorn3 The changes in previous pr is now here.
r? @nagisa

@crlf0710 crlf0710 mentioned this pull request Apr 30, 2021
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 30, 2021
@crlf0710 crlf0710 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 30, 2021
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from 231b44c to d84848b Compare April 30, 2021 14:06
@crlf0710 crlf0710 mentioned this pull request Apr 30, 2021
Closed
5 tasks
@nagisa
Copy link
Member

nagisa commented May 1, 2021

Introducing a flag looks like a pretty okay approach, but I'm not a fan of how or where the flag is set in the first place. It seems to me that setting the is_primary flag after all of the partitioning work is done would be better. At the very least the code around preserving the flag as the codegen units are replaced with others or merged seems quite prone to mistakes in the future.

@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from d84848b to f6e3335 Compare May 8, 2021 16:33
@crlf0710
Copy link
Member Author

crlf0710 commented May 8, 2021

@nagisa Thanks for the review! I addressed the comments above.

nagisa
nagisa reviewed May 8, 2021
View reviewed changes
Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

A couple nits. r=me once resolved.

compiler/rustc_codegen_ssa/src/base.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/mir/mono.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/mir/mono.rs Outdated Show resolved Hide resolved
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from f6e3335 to f84b26d Compare May 9, 2021 02:41
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the codegen_nonlocal_main_wrapper branch from f84b26d to 89a6705 Compare May 9, 2021 02:52
@crlf0710
Copy link
Member Author

crlf0710 commented May 9, 2021

@nagisa Review comments addressed~

@nagisa
Copy link
Member

nagisa commented May 9, 2021

@bors r+ thanks!

@bors
Copy link
Contributor

bors commented May 9, 2021

📌 Commit 89a6705 has been approved by nagisa

@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 May 9, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 9, 2021
@Dylan-DPC
Rollup merge of rust-lang#84507 - crlf0710:codegen_nonlocal_main_wrap…
28ce404 
…per, r=nagisa

Add primary marker on codegen unit and generate main wrapper on primary codegen.

This is the codegen part of changes extracted from rust-lang#84062.

This add a marker called `primary` on each codegen units, where exactly one codegen unit will be `primary = true` at a time. This specific codegen unit will take charge of generating `main` wrapper when `main` is imported from a foreign crate after the implementation of RFC 1260.

cc rust-lang#28937

I'm not sure who should i ask for review for codegen changes, so feel free to reassign.
r? `@nagisa`
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request May 9, 2021
Closed
@bors
Copy link
Contributor

bors commented May 10, 2021

⌛ Testing commit 89a6705 with merge d29289c...

@bors
Copy link
Contributor

bors commented May 10, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing d29289c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 10, 2021
@bors bors merged commit d29289c into rust-lang:master May 10, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 10, 2021
@crlf0710 crlf0710 deleted the codegen_nonlocal_main_wrapper branch May 10, 2021 03:26
@crlf0710
Copy link
Member Author

This has landed. @bjorn3

@bjorn3
Copy link
Member

bjorn3 commented May 10, 2021

Thanks. I will update cg_clif tomorrow when the next nightly is released.

@bjorn3 bjorn3 mentioned this pull request May 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagisa nagisa nagisa left review comments

Assignees

@nagisa nagisa

Labels
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.54.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@crlf0710 @rust-log-analyzer @bors @nagisa @bjorn3 @rustbot