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.

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

Use object crate for .rustc metadata generation #91604

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Dec 6, 2021

We already use the object crate for generating uncompressed .rmeta
metadata object files. This switches the generation of compressed
.rustc object files to use the object crate as well. These have
slightly different requirements in that .rmeta should be completely
excluded from any final compilation artifacts, while .rustc should
be part of shared objects, but not loaded into memory.

The primary motivation for this change is #90326: In LLVM 14, the
current way of setting section flags (and in particular, preventing
the setting of SHF_ALLOC) will no longer work. There are other ways
we could work around this, but switching to the object crate seems
like the most elegant, as we already use it for .rmeta, and as it
makes this independent of the codegen backend. In particular, we
don't need separate handling in codegen_llvm and codegen_gcc.
codegen_cranelift should be able to reuse the implementation as
well, though I have omitted that here, as it is not based on
codegen_ssa.

This change mostly extracts the existing code for .rmeta handling
to allow using it for .rustc as well, and adjusts the codegen
infrastructure to handle the metadata object file separately: We
no longer create a backend-specific module for it, and directly
produce the compiled module instead.

This does not fix #90326 by itself yet, as .llvmbc will need to be
handled separately.

r? @nagisa

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_gcc

cc @antoyo

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2021
@nagisa
Copy link
Member

nagisa commented Dec 6, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 6, 2021
@bors
Copy link
Contributor

bors commented Dec 6, 2021

⌛ Trying commit 3e1e7830dac9cc37d464a83b358e8bfeef6bf9b1 with merge 4b464f70d4bb56629e0b06964fcba1d010c0abfd...

@bors
Copy link
Contributor

bors commented Dec 6, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2021
@rust-log-analyzer

This comment has been minimized.

We already use the object crate for generating uncompressed .rmeta
metadata object files. This switches the generation of compressed
.rustc object files to use the object crate as well. These have
slightly different requirements in that .rmeta should be completely
excluded from any final compilation artifacts, while .rustc should
be part of shared objects, but not loaded into memory.

The primary motivation for this change is rust-lang#90326: In LLVM 14, the
current way of setting section flags (and in particular, preventing
the setting of SHF_ALLOC) will no longer work. There are other ways
we could work around this, but switching to the object crate seems
like the most elegant, as we already use it for .rmeta, and as it
makes this independent of the codegen backend. In particular, we
don't need separate handling in codegen_llvm and codegen_gcc.
codegen_cranelift should be able to reuse the implementation as
well, though I have omitted that here, as it is not based on
codegen_ssa.

This change mostly extracts the existing code for .rmeta handling
to allow using it for .rustc as well, and adjust the codegen
infrastructure to handle the metadata object file separately: We
no longer create a backend-specific module for it, and directly
produce the compiled module instead.

This does not fix rust-lang#90326 by itself yet, as .llvmbc will need to be
handled separately.
@nikic
Copy link
Contributor Author

nikic commented Dec 7, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Dec 7, 2021

⌛ Trying commit 9488cac with merge 5209ab525db4ef738b98e65d6d5f45ee441de18f...

@bors
Copy link
Contributor

bors commented Dec 7, 2021

☀️ Try build successful - checks-actions
Build commit: 5209ab525db4ef738b98e65d6d5f45ee441de18f (5209ab525db4ef738b98e65d6d5f45ee441de18f)

@rust-timer
Copy link
Collaborator

Queued 5209ab525db4ef738b98e65d6d5f45ee441de18f with parent 2af5c65, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5209ab525db4ef738b98e65d6d5f45ee441de18f): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

  • Large regression in instruction counts (up to 4.1% on incr-unchanged builds of deep-vector)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 7, 2021
@nikic
Copy link
Contributor Author

nikic commented Dec 7, 2021

I don't get this regression on deep-vector. Compressed metadata is only relevant for dylibs, while this is an executable. The time diff is reported in monomorphization_collector_graph_walk, which doesn't really look related. I'm confused.

@Mark-Simulacrum
Copy link
Member

It's almost certainly due to rust-lang/rustc-perf#1105, I wouldn't worry about the incr-unchanged regression for deep-vector.

@nagisa
Copy link
Member

nagisa commented Dec 7, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 7, 2021

📌 Commit 9488cac 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. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 7, 2021
@bors
Copy link
Contributor

bors commented Dec 8, 2021

⌛ Testing commit 9488cac with merge f9e77f2...

@bors
Copy link
Contributor

bors commented Dec 8, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 8, 2021
@bors bors merged commit f9e77f2 into rust-lang:master Dec 8, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 8, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f9e77f2): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@rustbot rustbot removed the perf-regression Performance regression. label Dec 8, 2021
BinaryFormat::Elf
};

let mut file = write::Object::new(binary_format, architecture, endianness);
Copy link
Member

Choose a reason for hiding this comment

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

This should probably get a file.add_file_symbol(...) call. In cg_clif I used the cgu name as argument.

bjorn3 added a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Dec 9, 2021
@bjorn3
Copy link
Member

bjorn3 commented Dec 9, 2021

Updated cg_clif in bjorn3/rustc_codegen_cranelift@4796207.

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Dec 31, 2021
Use object crate for .rustc metadata generation

We already use the object crate for generating uncompressed .rmeta
metadata object files. This switches the generation of compressed
.rustc object files to use the object crate as well. These have
slightly different requirements in that .rmeta should be completely
excluded from any final compilation artifacts, while .rustc should
be part of shared objects, but not loaded into memory.

The primary motivation for this change is rust-lang#90326: In LLVM 14, the
current way of setting section flags (and in particular, preventing
the setting of SHF_ALLOC) will no longer work. There are other ways
we could work around this, but switching to the object crate seems
like the most elegant, as we already use it for .rmeta, and as it
makes this independent of the codegen backend. In particular, we
don't need separate handling in codegen_llvm and codegen_gcc.
codegen_cranelift should be able to reuse the implementation as
well, though I have omitted that here, as it is not based on
codegen_ssa.

This change mostly extracts the existing code for .rmeta handling
to allow using it for .rustc as well, and adjusts the codegen
infrastructure to handle the metadata object file separately: We
no longer create a backend-specific module for it, and directly
produce the compiled module instead.

This does not `fix` rust-lang#90326 by itself yet, as .llvmbc will need to be
handled separately.

r? `@nagisa`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

10 participants