Skip to content

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Oct 4, 2025

This is mainly motivated by #142897, which proposes to move the LLVM FFI bindings out of rustc_codegen_llvm and into rustc_llvm, which is arguably the more correct place for them from a linking perspective.


In order to perform that migration, all of the types used in FFI signatures also need to be moved. However, several of those types have inherent methods that convert from backend-independent types to LLVM FFI types.

Moving the inherent methods as-is would require adding a lot of otherwise-unnecessary dependencies to rustc_llvm. And we can't leave them behind as-is, because inherent methods can't be defined in another crate.

Therefore, this PR replaces several of those inherent methods with either extension trait methods or free functions.

Using a helper trait allows the conversions to remain in `rustc_codegen_llvm`,
even if the FFI types are moved to a different crate.
This patch also moves `Regions` to a different module, since that type can stay
put when the FFI bindings move to another crate.
@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 4, 2025

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@Zalathar
Copy link
Contributor Author

Zalathar commented Oct 4, 2025

cc @cuviper as reviewer of #142897.

I ended up making some slightly different choices from the ones that PR, based on my own experience with LLVM bindings.

@Zalathar
Copy link
Contributor Author

Zalathar commented Oct 4, 2025

This will have import conflicts with #147322; I'll deal with them as they arise.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

slightly different set of changes but morally identical. thanks!

View changes since this review

Comment on lines +665 to +667
/// Determines the appropriate [`llvm::CallConv`] to use for a given function
/// ABI, for the current target.
pub(crate) fn to_llvm_calling_convention(sess: &Session, abi: CanonAbi) -> llvm::CallConv {
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, this can't use FromGeneric because it doesn't fit the signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, it would have the same signature if CanonAbi preserved a distinction between Nvidia and AMD.

I made a draft patch to do that, but ultimately decided it was out of scope for this PR.

@workingjubilee
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Oct 5, 2025

📌 Commit f8c54d2 has been approved by workingjubilee

It is now in the queue for this repository.

@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 Oct 5, 2025
@workingjubilee
Copy link
Member

FromGeneric sounds slightly too... uh... generic... in the "might get confused with other meanings of the word 'generic'" way... but I don't have a good alternative suggestion.

bors added a commit that referenced this pull request Oct 5, 2025
Rollup of 4 pull requests

Successful merges:

 - #147327 (cg_llvm: Remove inherent methods from several LLVM FFI types)
 - #147332 (Set opt-level flag for installing tool only on CI)
 - #147374 (bootstrap: don't build book redirect pages during dry-run/test)
 - #147376 (bootstrap: relax `compiler-rt` root assertion)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d8fc465 into rust-lang:master Oct 5, 2025
10 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Oct 5, 2025
rust-timer added a commit that referenced this pull request Oct 5, 2025
Rollup merge of #147327 - Zalathar:inherent, r=workingjubilee

cg_llvm: Remove inherent methods from several LLVM FFI types

This is mainly motivated by #142897, which proposes to move the LLVM FFI bindings out of `rustc_codegen_llvm` and into `rustc_llvm`, which is arguably the more correct place for them from a linking perspective.

---

In order to perform that migration, all of the types used in FFI signatures also need to be moved. However, several of those types have inherent methods that convert from backend-independent types to LLVM FFI types.

Moving the inherent methods as-is would require adding a lot of otherwise-unnecessary dependencies to `rustc_llvm`. And we can't leave them behind as-is, because inherent methods can't be defined in another crate.

Therefore, this PR replaces several of those inherent methods with either extension trait methods or free functions.
@Zalathar Zalathar deleted the inherent branch October 5, 2025 22:19
@Zalathar
Copy link
Contributor Author

Zalathar commented Oct 5, 2025

I don’t love FromGeneric, and considered some other possibilities.

In the end, I settled on preserving the existing name, mainly so that I could stop thinking about it and move on with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants