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

Autodiff Upstreaming - rustc_codegen_llvm changes #130060

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

ZuseZ4
Copy link
Contributor

@ZuseZ4 ZuseZ4 commented Sep 7, 2024

Now that the autodiff/Enzyme backend is merged, this is an upstream PR for the rustc_codegen_llvm changes.
It also includes small changes to three files under compiler/rustc_ast, which overlap with my frontend PR (#129458).
Here I only include minimal definitions of structs and enums to be able to build this backend code.
The same goes for minimal changes to compiler/rustc_codegen_ssa, the majority of changes there will be in another PR, once either this or the frontend gets merged.

We currently have 68 files left to merge, 19 in the frontend PR, 21 (+3 from the frontend) in this PR, and then ~30 in the middle-end.

This PR is large because it includes two of my three large files (~800 loc each). I could also first only upstream enzyme_ffi.rs, but I think people might want to see some use of these bindings in the same PR?

To already highlight the things which reviewers might want to discuss:

  1. enzyme_ffi.rs: I do have a fallback module to make sure that we don't link rustc against Enzyme when we build rustc without autodiff support.

  2. add_panic_msg_to_global was a pain to write and I currently can't even use it. Enzyme writes gradients into shadow memory. Pass in one float scalar? We'll allocate and return an extra float telling you how this float affected the output. Pass in a slice of floats? We'll let you allocate the vector and pass in a mutable reference to a float slice, we'll then write the gradient into that slice. It should be at least as large as your original slice, so we check that and panic if not. Currently we panic silently, but I already generate a nicer panic message with this function. I just don't know how to print it to the user. yet. I discussed this with a few rustc devs and the best we could come up with (for now), was to look for mangled panic calls in the IR and pick one, which works surprisingly reliably. If someone knows a good way to clean this up and print the panic message I'm all in, otherwise I can remove the code that writes the nicer panic message and keep the silent panic, since it's enough for soundness. Especially since this PR is already a bit larger.

  3. SanitizeHWAddress: When differentiating C++, Enzyme can use TBAA to "understand" enums/unions, but for Rust we don't have this information. LLVM might to speculative loads which (without TBAA) confuse Enzyme, so we disable those with this attribute. This attribute is only set during the first opt run before Enzyme differentiates code. We then remove it again once we are done with autodiff and run the opt pipeline a second time. Since enums are everywhere in Rust, support for them is crucial, but if this looks too cursed I can remove these ~100 lines and keep them in my fork for now, we can then discuss them separately to make this PR simpler?

  4. Duplicated llvm-opt runs: Differentiating already optimized code (and being able to do additional optimizations on the fly, e.g. for GPU code) is the reason why Enzyme is so fast, so the compile time is acceptable for autodiff users: https://enzyme.mit.edu/talks/Publications/ (There are also algorithmic issues in Enzyme core which are more serious than running opt twice).

  5. I assume that if we merge these minimal cg_ssa changes here already, I also need to fix the other backends (GCC and cliff) to have dummy implementations, correct?

  6. I'm happy to split this PR up further if reviewers have recommendations on how to.

For the full implementation, see: #129175

Tracking:

@rustbot
Copy link
Collaborator

rustbot commented Sep 7, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
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
Copy link
Collaborator

rustbot commented Sep 7, 2024

⚠️ Warning ⚠️

  • These commits modify submodules.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 7, 2024

This PR modifies config.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

@rust-log-analyzer

This comment has been minimized.

@@ -176,6 +176,8 @@ pub(crate) fn default_configuration(sess: &Session) -> Cfg {
// NOTE: These insertions should be kept in sync with
// `CheckCfg::fill_well_known` below.

ins_none!(sym::autodiff_fallback);
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be insta stable, it should be at least gated behind nightly compiler.

Suggested change
ins_none!(sym::autodiff_fallback);
if sess.is_nightly_build() {
ins_none!(sym::autodiff_fallback);
}

Please also follow all the steps regarding a new cfg as defined in the top of this file (as well as the tests files):

//! ## Adding a new cfg
//!
//! Adding a new feature requires two new symbols one for the cfg it-self
//! and the second one for the unstable feature gate, those are defined in
//! `rustc_span::symbol`.
//!
//! As well as the following points,
//! - Add the activation logic in [`default_configuration`]
//! - Add the cfg to [`CheckCfg::fill_well_known`] (and related files),
//! so that the compiler can know the cfg is expected
//! - Add the cfg in [`disallow_cfgs`] to disallow users from setting it via `--cfg`
//! - Add the feature gating in `compiler/rustc_feature/src/builtin_attrs.rs`

@jieyouxu jieyouxu added the F-autodiff `#![feature(autodiff)]` label Sep 7, 2024
@fee1-dead
Copy link
Member

r? compiler

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@michaelwoerister
Copy link
Member

r? compiler

@rustbot rustbot assigned davidtwco and unassigned michaelwoerister Oct 7, 2024
@davidtwco
Copy link
Member

There's very little chance of this being merged in one PR with one commit of this size. You'll need to split this up into well-commented/motivated PRs that can be landed one at a time. I haven't spent much time looking at this PR, so I don't have any suggestions on how to split this up. I'd recommend finding someone on the compiler team who is interested in these changes and who you can work with to do the reviews.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

I don't have time to review this, so just one drive-by note: It looks like a decent part of the extra FFI APIs in enzyme_ffi.rs are essentially duplicates of things that we already have bindings for under slightly different names and signatures. Like we already have LLVMRustAddFunctionAttributes/LLVMRustAddCallSiteAttributes and this introduces LLVMRustAddEnumAttributeAtIndex. It also looks like the code doesn't make use of the Builder abstraction and instead calls FFI APIs directly everywhere, which is probably also where the duplication comes from.

@davidtwco davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2024
@alex-semenyuk alex-semenyuk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 13, 2024
@ZuseZ4 ZuseZ4 force-pushed the enzyme-cg-llvm branch 3 times, most recently from 11c3bae to 78297a9 Compare November 21, 2024 01:21
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2024
@bors

This comment was marked as resolved.

oli-obk

This comment was marked as resolved.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 1, 2025

bors r plus

@bors
Copy link
Contributor

bors commented Jan 1, 2025

📌 Commit 4895a33 has been approved by oli-obk

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 1, 2025
@rust-log-analyzer

This comment has been minimized.

Comment on lines 187 to 191
// pub fn LLVMRustVerifyFunction(V: &Value, action: LLVMRustVerifierFailureAction) -> Bool;
extern "C" bool LLVMRustVerifyFunction(LLVMValueRef Fn,
LLVMRustVerifierFailureAction Action) {
//Function *F = unwrap<Function>(Fn);
return LLVMVerifyFunction(Fn, fromRust(Action));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Uhh, sorry, but looks like after rebase this reverted back to wrong LLVMBool\bool types. And stray commendted out code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I also formated the c++ file.

@ZuseZ4
Copy link
Contributor Author

ZuseZ4 commented Jan 1, 2025

thank you to everyone for the various rounds of review!

@traviscross
Copy link
Contributor

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jan 1, 2025

📌 Commit d753cbf has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 2, 2025

⌛ Testing commit d753cbf with merge 504f4f5...

@bors
Copy link
Contributor

bors commented Jan 2, 2025

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 504f4f5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 2, 2025
@bors bors merged commit 504f4f5 into rust-lang:master Jan 2, 2025
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Jan 2, 2025
@bors bors mentioned this pull request Jan 2, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (504f4f5): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.2% [-0.3%, -0.2%] 2
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 0.2%, secondary -2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.8% [0.8%, 2.8%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.9% [-2.9%, -2.9%] 1
Improvements ✅
(secondary)
-2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) 0.2% [-2.9%, 2.8%] 3

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 762.325s -> 764.563s (0.29%)
Artifact size: 325.51 MiB -> 325.55 MiB (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-spurious-x86_64-apple-SIGSEGV-SIGILL CI spurious failure: SIGSEGV/SIGILL while building rustc itself on x86_64-apple F-autodiff `#![feature(autodiff)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.