-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Port rustc codegen attrs #151336
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
Port rustc codegen attrs #151336
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_hir/src/attrs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
| impl<S: Stage> NoArgsAttributeParser<S> for RustcOffloadKernelParser { | ||
| const PATH: &[Symbol] = &[sym::rustc_offload_kernel]; | ||
| const ON_DUPLICATE: OnDuplicate<S> = OnDuplicate::Error; | ||
| const ALLOWED_TARGETS: AllowedTargets = AllowedTargets::AllowList(&[Allow(Target::ForeignFn)]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From #147936 it seems like Target::Fn should also be allowed. I don't think ForeignFn should be allowed. Note that an extern fn { ... } is still just a Target::Fn
This attribute does not seem to have a single test or usage in rustc which is fascinating
@Sa4dUs As the implementer of this attribute, what should the correct target list be? Only functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute does not seem to have a single test or usage in rustc which is fascinating
I also found this fascinating, I thought that extern fn would be Target::ForeignFn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust has two related concepts:
A normal function that is exported with an abi:
extern "ABI" fn thing {
}
A foreign function:
extern "ABI" {
fn thing();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, accepting only Target:Function should be the way to go. this attribute is currently used to adjust functions written in rust to be compatible with offload.
we'll add some tests using this attr soon :)
cc @ZuseZ4 in case you have in mind any case where we might want to mark a ForeignFn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this intrinsic it doesn't really make sense to support ForeignFn. It is there to run Rust code on a GPU.
Our other intrinsic, offload_args however only realy makes sense in combination with a ForeignFn. See e.g. https://github.com/rust-lang/rust/pull/150683/changes#diff-e92c562c10044d53fd41b1b06ef59a43087a6434e09fa8e667dea5b729d07726
However, I would expect that in most cases, people offload a safe wrapper around the ForeignFn (like I'm doing it in my test case there), instead of directly offloading the ForeignFn. So I think it should be fine to not allow ForeignFn at the moment, and only discuss it if users complain.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ecc0410 to
b52ecfe
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me after solving conflicts
@bors rollup d+
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@bors delegate |
|
✌️ @Bryntet, you can now approve this pull request! If @JonathanBrouwer told you to " |
b52ecfe to
3e731f7
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r=JonathanBrouwer |
Rollup merge of #151336 - port_rustc_codegen_attrs, r=JonathanBrouwer Port rustc codegen attrs Tracking issue: #131229 two more quick ones r? @JonathanBrouwer
…onathanBrouwer Port `#[patchable_function_entry]` to attr parser This is the last codegen attr (see rust-lang#151335 and rust-lang#151336)! Tracking issue: rust-lang#131229 currently this PR is rebased on rust-lang#151336 to make CI pass for the last commit to see the actual changes in this PR you can look [here](https://github.com/rust-lang/rust/pull/151340/changes/3e731f7e84301a898a36e46ee5e4845ff9bda98a..55111fb468808b733e97170a841217a67666ac33)
Rollup merge of #151340 - port_patchable_function_entry, r=JonathanBrouwer Port `#[patchable_function_entry]` to attr parser This is the last codegen attr (see #151335 and #151336)! Tracking issue: #131229 currently this PR is rebased on #151336 to make CI pass for the last commit to see the actual changes in this PR you can look [here](https://github.com/rust-lang/rust/pull/151340/changes/3e731f7e84301a898a36e46ee5e4845ff9bda98a..55111fb468808b733e97170a841217a67666ac33)
Tracking issue: #131229
two more quick ones
r? @JonathanBrouwer