-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 - enzyme frontend #129458
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #129563) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
Thanks for the work on autodiff! I have some feedback and questions. As you may have gathered, I am not very knowledgeable about autodiff. If I ask some questions for more context / explanation, it might be good to encode them as comments in the impl itself for more context. So that if someone else (or even yourself) comes back later to try to change this impl, they are better equipped to figure out what this is doing.
EDIT: please ignore panic!
-> bug!
suggestions as that might not be available yet in macro expansion here.
9d8ec28
to
a989f28
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@jieyouxu Now that the test infra has landed, is there anything left to do except updating the tests? |
a37ca8e
to
8ea96f9
Compare
I added the autodiff gating for all test files and moved the failing one to UI, they are not executed now without AD. I also just added my former collaborator as co-author to the PR, so from my side it's ready. |
This comment has been minimized.
This comment has been minimized.
8ea96f9
to
c3f225f
Compare
This comment has been minimized.
This comment has been minimized.
c3f225f
to
14dc8b1
Compare
a6fec80
to
f86bed6
Compare
There isn't a field to answer, so I'll say it here, yep I made two modules out of it @jieyouxu : #129458 (comment) |
☔ The latest upstream changes (presumably #131237) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
Thanks for the updates, this looks to be in a much better shape than before. I've left a ~final round of reviews, and this prototype looks like it's suitable for merge after addressing those. The diagnostics for invalid attributes seem somewhat rough, but the polishing can be improved in the future and is not blocking. The test coverage regarding to all the branches in the macro logic isn't very thorough, but that's fine for initial upstreaming of the prototype for now (but definitely will need to be improved if this ever wants to be stabilized).
tests/ui/autodiff_illegal.rs
Outdated
// We would like to support this case in the future | ||
#[autodiff(df18, Reverse, Active, Active)] | ||
fn f18(x: F64Trans) -> f64 { | ||
//~^^ ERROR failed to resolve: use of undeclared type `F64Trans` [E0433] |
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.
Remark: "failed to resolve" is a strange error here?
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.
Ah, I missed answering that. Generally agreed, but as long as it errors I'm fine as it's one of the things that will require TypeInfo to catch all invalid usage combinations. Then again I think that if it wouldn't error Enzyme would handle it correctly, so if this bug magically disappeared then I can probably remove the error message and move it to the pretty tests.
205fc71
to
8371b60
Compare
This comment has been minimized.
This comment has been minimized.
8371b60
to
3c01533
Compare
Co-authored-by: Lorenz Schmidt <bytesnake@mailbox.org>
3c01533
to
59685c4
Compare
@jieyouxu the last force-push only updated docs (for the -1 typetree info and to include an example of how expansion will look like), so I will assume that this should pass CI again. As before, let me know if I missed something, but I think I should have everything covered this time. Thanks a lot for taking the time to review this. The middle end should be fairly small from here on, and the rustc_cg_llvm changes are fairly big, but are also pretty straightforward due to including bindings and only forwarding rust configs to Enzyme configs (even though reviewers currently don't seem to be keen on it), so hopefully they will be a bit easier to handle. |
👍 That plan sounds fair to me. |
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 looks mostly good to me. I just noticed one thing, please add a feature gate ui test to make sure we don't ICE or do funny things.
#![crate_type = "lib"]
#[autodiff]
//~^ ERROR cannot find attribute `autodiff` in this scope
//~| ERROR: `#[autodiff]` is experimental
fn foo() {}
e.g.
#[doc(notable_trait)] //~ ERROR: `#[doc(notable_trait)]` is experimental |
This is an upstream PR for the
autodiff
rustc_builtin_macro that is part of the autodiff feature.For the full implementation, see: #129175
Content:
It contains a new
#[autodiff(<args>)]
rustc_builtin_macro, as well as a#[rustc_autodiff]
builtin attribute.The autodiff macro is applied on function
f
and will expand to a second functiondf
(name given by user).It will add a dummy body to
df
to make sure it type-checks. The body will later be replaced by enzyme on llvm-ir level,we therefore don't really care about the content. Most of the changes (700 from 1.2k) are in
compiler/rustc_builtin_macros/src/autodiff.rs
, which expand the macro. Nothing except expansion is implemented for now.I have a fallback implementation for relevant functions in case that rustc should be build without autodiff support. The default for now will be off, although we want to flip it later (once everything landed) to on for nightly. For the sake of CI, I have flipped the defaults, I'll revert this before merging.
Dummy function Body:
The first line is an
inline_asm
nop to make inlining less likely (I have additional checks to prevent this in the middle end of rustc. Iff
gets inlined too early, we can't pass it to enzyme and thus can't differentiate it.If
df
gets inlined too early, the call site will just compute this dummy code instead of the derivatives, a correctness issue. The following black_box lines make sure that none of the input arguments is getting optimized away before we replace the body.Motivation:
The user facing autodiff macro can verify the user input. Then I write it as args to the rustc_attribute, so from here on I can know that these values should be sensible. A rustc_attribute also turned out to be quite nice to attach this information to the corresponding function and carry it till the backend.
This is also just an experiment, I expect to adjust the user facing autodiff macro based on user feedback, to improve usability.
As a simple example of what this will do, we can see this expansion:
From:
to
I will add a few more tests once I figured out why rustc rebuilds every time I touch a test.
Tracking: