compiler: Use LLVM's Comdat support by workingjubilee · Pull Request #131876 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation21 Commits3 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Some changes occurred in coverage instrumentation.
cc @Zalathar
Area: Source-based code coverage (-Cinstrument-coverage)
Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.
labels
These commits modify compiler targets.
They really don't they just touch the public API...
Thanks. r=me after squishing the fixups.
📌 Commit 49bb96c has been approved by Zalathar
It is now in the queue for this repository.
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
Comment on lines -1661 to -1669
| extern "C" void LLVMRustSetComdat(LLVMModuleRef M, LLVMValueRef V, |
|---|
| const char *Name, size_t NameLen) { |
| Triple TargetTriple = Triple(unwrap(M)->getTargetTriple()); |
| GlobalObject *GV = unwrap(V); |
| if (TargetTriple.supportsCOMDAT()) { |
| StringRef NameRef(Name, NameLen); |
| GV->setComdat(unwrap(M)->getOrInsertComdat(NameRef)); |
| } |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaict the LLVM-C API doesn't have an exact match of the support check we do here, which is why I implemented it via Rust. I'll go fix that.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
…kingjubilee
Rollup of 8 pull requests
Successful merges:
- rust-lang#127462 (std: uefi: Add basic Env variables)
- rust-lang#131537 (Fix range misleading field access)
- rust-lang#131838 (bootstrap: allow setting
--jobsin config.toml) - rust-lang#131871 (x86-32 float return for 'Rust' ABI: treat all float types consistently)
- rust-lang#131876 (compiler: Use LLVM's Comdat support)
- rust-lang#131890 (Update
usekeyword docs to describe precise capturing) - rust-lang#131899 (Mark unexpected variant res suggestion as having placeholders)
- rust-lang#131908 (rustdoc: Switch from FxHash to sha256 for static file hashing.)
r? @ghost
@rustbot modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
Comment on lines +181 to +184
| let name_buf = get_value_name(val).to_vec(); |
|---|
| let name = |
| CString::from_vec_with_nul(name_buf).or_else(|buf |
| set_comdat(llmod, val, &name); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, looks like we need the supports-comdat check here too.
Or maybe this is a sign that the check should be inside set_comdat instead?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this because !self.is_like_aix && !self.is_like_osx should be !self.is_like_aix || !self.is_like_osx?
No.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean it's really a sign that "setting a comdat" needs to be a function on the CodegenCx, imo, where we already have acquired all the info, so that it isn't called when we don't have access to that.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense. Happy to leave that for future work.
Migrate llvm::set_comdat and llvm::SetUniqueComdat to LLVM-C FFI.
Note, now we can call llvm::set_comdat only when the target actually
supports adding comdat. As this has no convenient LLVM-C API, we
implement this as TargetOptions::supports_comdat.
Co-authored-by: Stuart Cook Zalathar@users.noreply.github.com
Zalathar added a commit to Zalathar/rust that referenced this pull request
This succeeded in aarch64-apple, so let's try it again.
@bors r+
📌 Commit 4927600 has been approved by Zalathar
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
Rollup of 4 pull requests
Successful merges:
- rust-lang#131876 (compiler: Use LLVM's Comdat support)
- rust-lang#131941 (compiletest: disambiguate html-tidy from rust tidy tool)
- rust-lang#131942 (compiler: Adopt rust-analyzer impls for
LayoutCalculatorError) - rust-lang#131945 (rustdoc: Clean up footnote handling)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…nem,workingjubilee
Make llvm::set_section take a &CStr
There's no reason to convert the section name to an intermediate String, when the LLVM-C API wants a C string anyway.
Follow-up to rust-lang#131876.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…nem,workingjubilee
Make llvm::set_section take a &CStr
There's no reason to convert the section name to an intermediate String, when the LLVM-C API wants a C string anyway.
Follow-up to rust-lang#131876.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#131962 - Zalathar:llvm-set-section, r=Swatinem,workingjubilee
Make llvm::set_section take a &CStr
There's no reason to convert the section name to an intermediate String, when the LLVM-C API wants a C string anyway.
Follow-up to rust-lang#131876.
Labels
Area: Source-based code coverage (-Cinstrument-coverage)
Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.