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

Add link attribute for Enzyme's LLVMRust FFI #136374

Merged
merged 1 commit into from
Feb 1, 2025

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Feb 1, 2025

Since #133429 landed, the compiler doesn't build with -Zcross-crate-inline-threshold=always. I don't expect anyone else to test or fix issues with that goofy configuration, so I'm fixing it.

This PR adds a link attribute just like #118142 for all the new LLVMRust functions. They were actually added in #130060 but weren't used until just now.

@rustbot
Copy link
Collaborator

rustbot commented Feb 1, 2025

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
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

@rustbot rustbot added 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 Feb 1, 2025
@saethlin saethlin changed the title Add link attribute for Enzyme's FFI Add link attribute for Enzyme's LLVMRust FFI Feb 1, 2025
@ZuseZ4
Copy link
Contributor

ZuseZ4 commented Feb 1, 2025

cc @oli-obk I'll look again at the code over the weekend. Maybe it's the query we discussed?
Sorry for breaking your config

@lqd
Copy link
Member

lqd commented Feb 1, 2025

That enzyme PR had a few perf regressions, I don’t know if it needs more time to iron these issues out. Maybe we should wait a bit and see what happens there to know when to land this one? Apart from that it looks good to me.

@ZuseZ4
Copy link
Contributor

ZuseZ4 commented Feb 1, 2025

There are some lines which I can just gate against the enzyme cfg - by default we don't build enzyme, so they wouldn't be executed. I just didn't consider them having an impact. Can we bench that before reverting please? I can do that today.

@lqd
Copy link
Member

lqd commented Feb 1, 2025

Oh for sure! If there’s a fix, it would be better than doing a revert. (This PR is a fix for the linkage issue though, let’s keep the perf discussion in #133429)

@oli-obk
Copy link
Contributor

oli-obk commented Feb 1, 2025

r? oli-obk

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 1, 2025

📌 Commit ce7cb31 has been approved by oli-obk

It is now in the queue for this repository.

@rustbot rustbot assigned oli-obk and unassigned BoxyUwU Feb 1, 2025
@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 Feb 1, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2025
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#130514 (Implement MIR lowering for unsafe binders)
 - rust-lang#135684 (docs: Documented Send and Sync requirements for Mutex + MutexGuard)
 - rust-lang#136307 (Implement all mix/max functions in a (hopefully) more optimization amendable way)
 - rust-lang#136360 (Stabilize `once_wait`)
 - rust-lang#136364 (document that ptr cmp is unsigned)
 - rust-lang#136374 (Add link attribute for Enzyme's LLVMRust FFI)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2025
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#130514 (Implement MIR lowering for unsafe binders)
 - rust-lang#135684 (docs: Documented Send and Sync requirements for Mutex + MutexGuard)
 - rust-lang#136307 (Implement all mix/max functions in a (hopefully) more optimization amendable way)
 - rust-lang#136360 (Stabilize `once_wait`)
 - rust-lang#136364 (document that ptr cmp is unsigned)
 - rust-lang#136374 (Add link attribute for Enzyme's LLVMRust FFI)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2a82ebd into rust-lang:master Feb 1, 2025
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2025
Rollup merge of rust-lang#136374 - saethlin:enzyme-linkage, r=oli-obk

Add link attribute for Enzyme's LLVMRust FFI

Since rust-lang#133429 landed, the compiler doesn't build with `-Zcross-crate-inline-threshold=always`. I don't expect anyone else to test or fix issues with that goofy configuration, so I'm fixing it.

This PR adds a link attribute just like rust-lang#118142 for all the new LLVMRust functions. They were actually added in rust-lang#130060 but weren't used until just now.
@rustbot rustbot added this to the 1.86.0 milestone Feb 1, 2025
@saethlin saethlin deleted the enzyme-linkage branch February 1, 2025 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants