Skip to content

Add support for generics in offloadand remove no_mangle attribute - #159566

Open
Sa4dUs wants to merge 5 commits into
rust-lang:mainfrom
Sa4dUs:offload-generics
Open

Add support for generics in offloadand remove no_mangle attribute#159566
Sa4dUs wants to merge 5 commits into
rust-lang:mainfrom
Sa4dUs:offload-generics

Conversation

@Sa4dUs

@Sa4dUs Sa4dUs commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

implements a 3rd pass where we collect kernel instantiations required by offload to work
also enforces consistent mangling for offload functions between host and device

closes: #150985

r? @ZuseZ4

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 19, 2026
@Sa4dUs
Sa4dUs force-pushed the offload-generics branch from 7fefbf7 to a019aea Compare July 19, 2026 14:10
@rust-log-analyzer

This comment has been minimized.

Sign in to view
bjorn3
bjorn3 reviewed Jul 19, 2026
View reviewed changes
Comment thread compiler/rustc_codegen_ssa/src/back/symbol_export.rs
// core/std/allocators/etc. For example symbols used to hook up allocation
// are not considered for export
let codegen_fn_attrs = tcx.codegen_fn_attrs(sym_def_id);
let is_extern = codegen_fn_attrs.contains_extern_indicator();

@bjorn3 bjorn3 Jul 19, 2026 •
edited by rustbot
Loading

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.

Perhaps contains_extern_indicator should return true for offload functions?

View changes since the review

bjorn3
bjorn3 reviewed Jul 19, 2026
View reviewed changes
Comment thread compiler/rustc_monomorphize/src/collector.rs Outdated
bjorn3
bjorn3 reviewed Jul 19, 2026
View reviewed changes
Comment thread compiler/rustc_codegen_ssa/src/base.rs Outdated
@ZuseZ4

ZuseZ4 commented Jul 20, 2026

Copy link
Copy Markdown
Member

Great improvement ( I'll review later).
@Flakebi fyi, I think now we only have the abi checks left to implement for more complex types and offload should cover all the bigger things we talked about?

@rust-bors

This comment has been minimized.

Sign in to view
bjorn3
bjorn3 reviewed Jul 23, 2026
View reviewed changes
Comment thread compiler/rustc_codegen_ssa/src/back/write.rs Outdated
@Sa4dUs
Sa4dUs force-pushed the offload-generics branch from b40ecd1 to cb87656 Compare July 23, 2026 16:44
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Sa4dUs
Sa4dUs force-pushed the offload-generics branch from cb87656 to f684946 Compare July 24, 2026 13:07
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Sa4dUs
Sa4dUs marked this pull request as ready for review July 24, 2026 15:07
@rustbot

rustbot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 24, 2026
bjorn3
bjorn3 reviewed Jul 24, 2026
View reviewed changes
Comment thread compiler/rustc_monomorphize/src/collector.rs
bjorn3
bjorn3 reviewed Jul 24, 2026
View reviewed changes
Comment thread compiler/rustc_monomorphize/src/offload_manifest.rs
bjorn3
bjorn3 reviewed Jul 24, 2026
View reviewed changes
Comment thread compiler/rustc_monomorphize/src/offload_manifest.rs Outdated
bjorn3
bjorn3 reviewed Jul 24, 2026
View reviewed changes
Comment thread compiler/rustc_monomorphize/src/offload_manifest.rs Outdated
@bjorn3
bjorn3 self-requested a review July 24, 2026 20:49
@Flakebi

Flakebi commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Very nice!
If I read this correctly, there are two important parts here:

  1. Not adding the hash suffix when mangling offload kernel names makes the mangling deterministic
  2. Compilation first compiles the CPU code, which generates a new artifact – a list of function instances of kernels that are referenced. When compiling the GPU code afterwards, this list is added to the monomorphization roots.

On a sidenote: As due to 1., no more hash suffix is added to kernels, we should ensure that we fail compilation if someone tries to compile a program that defines the same mangled kernel name twice.
E.g. by importing two different versions of the same crate that defines the same kernel.
This might already fail to compile but a test for that would be good.

ZuseZ4
ZuseZ4 reviewed Jul 28, 2026
View reviewed changes

@ZuseZ4 ZuseZ4 left a comment •
edited by rustbot
Loading

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.

Just some smaller questions, at the end Oli should probably have another look. But other than cleaning up the old Host, can you add some actual tests for the generic functions?

View changes since this review

Comment thread compiler/rustc_monomorphize/src/collector.rs
.iter()
.any(|o| matches!(o, rustc_session::config::Offload::HostMetadata(_)));
if is_host_metadata {
let crate_items = tcx.hir_crate_items(());

@ZuseZ4 ZuseZ4 Jul 28, 2026

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.

What about trait_items? The impl_items note says it also covers traits, but he owners function on ModuleItems still collects both so they might not be exactly the same?

Comment thread compiler/rustc_session/src/config.rs
Device,
/// Like `Device`, but reads a manifest of required generic kernel instantiations
/// produced by a previous `HostMetadata` pass.
DeviceWithManifest(String),

@ZuseZ4 ZuseZ4 Jul 28, 2026

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.

Now we went from 2 to 4 stages :D Shouldn't we be able to drop the former Device variant? We could always just create an empty DeviceManifest, I think that's easier to understand than an extra compilation path.

Comment thread compiler/rustc_monomorphize/src/offload_manifest.rs
let Some(path) = tcx.sess.opts.unstable_opts.offload.iter().find_map(|o| {
if let rustc_session::config::Offload::HostMetadata(p) = o { Some(p) } else { None }
}) else {
return;

@ZuseZ4 ZuseZ4 Jul 28, 2026

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.

You already check that on the caller side, so maybe bug! here if you want to check again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZuseZ4 ZuseZ4 ZuseZ4 left review comments
@bjorn3 bjorn3 Awaiting requested review from bjorn3

At least 1 approving review is required to merge this pull request.

Assignees

@ZuseZ4 ZuseZ4

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

std::offload requires mangled names

6 participants

@Sa4dUs @rust-log-analyzer @ZuseZ4 @rustbot @Flakebi @bjorn3