-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Implement autodiff using intrinsics #142640
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
base: master
Are you sure you want to change the base?
Conversation
_ if tcx.has_attr(def_id, sym::rustc_autodiff) => { | ||
// NOTE(Sa4dUs): This is a hacky way to get the autodiff items | ||
// so we can focus on the lowering of the intrinsic call | ||
|
||
// `diff_items` is empty even when autodiff is enabled, and if we're here, | ||
// it's because some function was marked as intrinsic and had the `rustc_autodiff` attr | ||
let diff_items = tcx.collect_and_partition_mono_items(()).autodiff_items; | ||
|
||
// this shouldn't happen? | ||
if diff_items.is_empty() { | ||
bug!("no autodiff items found for {def_id:?}"); | ||
} | ||
|
||
// TODO(Sa4dUs): generate the enzyme call itself, based on the logic in `builder.rs` | ||
|
||
// Just gen the fallback body for now | ||
return Err(ty::Instance::new_raw(def_id, instance.args)); | ||
} |
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.
So this is the part you got stuck on?
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.
Kinda. I tried to use the collect_and_partition_mono_items
just as a hacky way of getting autodiff_items, so i can focus on the declaration. Once that was working, I would focus on how to get that information in the best way possible.
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.
Just for context, during yesterday's meeting, we came to the conclusion that, for the time being, it's acceptable to copy and paste the collector logic from within the provider's code, just to focus on the declaration, but still nice to have feedback since, in some moment, we'll need to get to AutoDiffItems
in a decent way. collect_and_partition_mono_items(()).autodiff_items
being []
in autodiff code seems weird, so it would be nice to know why it's happening, or if it is some kind of bug.
@Sa4dUs Can you provide the motivation in the PR desc for context? |
bfb72fd
to
fad0b0c
Compare
This comment has been minimized.
This comment has been minimized.
a72fd24
to
38b27a2
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.
tests/codegen/autodiff/batched.rs
Outdated
// CHECK-NEXT: %19 = insertvalue [4 x float] %17, float %18, 3 | ||
// CHECK-NEXT: ret [4 x float] %19 | ||
// CHECK-NEXT: } | ||
// CHECK-NEXT: %4 = fadd fast float %"_2'ipl", %"_2'ipl" |
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.
Is this test currently passing on master? Also, can you compare the IR of the two rustc versions just before we run Enzyme? I wonder if we call enzyme differently, or if just llvm changed. (Ignore this if you're still working on the code and are not done yet).
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, the test with the current fixes is passing at master. and the way we call enzyme is just the same, as i've based my work on the existing function for diff body codegen and removed unnecessary parts.
97f0985
to
43efce8
Compare
96f554c
to
d312894
Compare
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
Some changes occurred in compiler/rustc_codegen_llvm/src/builder/autodiff.rs cc @ZuseZ4 Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_builtin_macros/src/autodiff.rs cc @ZuseZ4 Some changes occurred in compiler/rustc_monomorphize/src/partitioning/autodiff.rs cc @ZuseZ4
|
r? codegen |
f1c7a40
to
c370d96
Compare
This comment has been minimized.
This comment has been minimized.
Note(Sa4dUs): `cg/generic.rs` test is passing with some tweaks
577cb5d
to
b7d7b73
Compare
This PR aims to move autodiff logic to
enzyme_autodiff
intrinsic. Allowing us to delete a great part of our frontend code and overall, simplify the compilation pipeline of autodiff functions.