Autodiff Upstreaming - rustc_codegen_ssa, rustc_middle #133429
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
To expand on 2) #[autodiff(bar, Reverse, ...)]
fn foo(x: f32) -> f32 { x*x }it will expand to #[rustc_autodiff]
fn foo(x: f32) -> f32 {x*x}
#[rustc_autodiff(Reverse,...)]
fn bar(x: f32, scalar_factor: f32) -> (f32, f32) {
// some_dummy_code()
}Now I have some logic in this PR which picks up the rustc_autodiff attributes and passes them onto the backend, where for every single |
7 tasks
jieyouxu
requested changes
Nov 25, 2024
| @@ -996,6 +997,35 @@ mod parse { | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) fn parse_autodiff(slot: &mut Vec<AutoDiff>, v: Option<&str>) -> bool { | |||
There was a problem hiding this comment.
Remark: this has no error messages if the autodiff options failed to parse, acceptable for unstable flag but still poor UX, unacceptable when it comes to stabilization time.
There was a problem hiding this comment.
It should be handled with a proper description in parse_autodiff above.
There was a problem hiding this comment.
I assume there is some rustc function to generate suggestions when users make a typo?
I'll update it for now to print the unrecognized value.
There was a problem hiding this comment.
I looked again at all the other options which accept more values (e.g. instrument_xray), and they have no error-handling either. Is there a preferred way to print errors here?
rustbot
added
S-waiting-on-author
For codegen/assembly/ui tests you can make it build w/ fat LTO via
I don't quite understand the implication of this. Is there some small example I can refer to?
Can you elaborate on what test conditions you need? What do you mean exactly by "isolated"? Can you not run autodiff but only if there is autodiff support, or do you mean like don't run by default even if there is autodiff support? |
|
https://github.com/rust-lang/rust/pull/130060/files#diff-a56b374664e290a55d70fa80e456b6280913830b382b73fb70c4483d3d4cf246 |
ZuseZ4
changed the title
9 tasks
This just means that for now, you'll have to gate autodiff-related tests with Note that this cannot break and should not modify code that does not use autodiff at all, which is indicated by codegen tests that do not use / opt-in to autodiff support. |
|
This PR modifies If appropriate, please update Some changes occurred in coverage instrumentation. cc @Zalathar Some changes occurred in cfg and check-cfg configuration cc @Urgau These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
Urgau
reviewed
Dec 16, 2024
ZuseZ4
force-pushed
the
autodiff-middle
branch
from
b30f1d7 to
2ad340e
Compare
This comment has been minimized.
This comment has been minimized.
|
I rebased now that the other autodiff PR got merged, fixed all conflicts, and got it to compile locally. |
|
☔ The latest upstream changes (presumably #136030) made this pull request unmergeable. Please resolve the merge conflicts. |
ZuseZ4
force-pushed
the
autodiff-middle
branch
from
2ad340e to
c4af0ba
Compare
This comment has been minimized.
This comment has been minimized.
|
@oli-obk Just to keep track, so far we have 3/4 things which should be fixed here. Potentially not all in this specific PR, but preferably before enabling it for default nightly builds.
After thinking about it for a bit I'll probably just do 4) for now, and leave 1-3 for a follow-up PR, just for the sake of having a working version upstream. I'll need to talk to jieyouxu to see if we then add test's already here, or in the follow-up PR where I fix 3). |
This comment has been minimized.
This comment has been minimized.
ZuseZ4
force-pushed
the
autodiff-middle
branch
from
1ab7aa0 to
950d91b
Compare
|
I'll need to add the fn-ptr error message and left a few questions. |
rustbot
added
the
S-waiting-on-review
rustbot
removed
the
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
oli-obk
requested changes
Jan 27, 2025
| It--; | ||
| //It--; |
There was a problem hiding this comment.
leftover commented out code from debugging (should be reinstated, right?)
There was a problem hiding this comment.
No, based on testing I actually have to delete It right there and not decrement, since it's a call instruction which uses all of the previous arguments and for llvm-ir you usually have to delete in reverse order, since it checks for remaining users. I deleted the line and renamed the function.
I'm very open to better names than LLVMRustEraseInstUntilInclusive
Comment on lines
1134
to
1152
| if ty.is_fn_ptr() { | ||
| unimplemented!("what to do whith fn ptr?"); | ||
| } |
There was a problem hiding this comment.
this is still unresolved. An unstructured tcx.span_err() is ok for this
rustbot
added
S-waiting-on-author
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Jan 27, 2025
Change `collect_and_partition_mono_items` tuple return type to a struct rust-lang#133429 will add a new field to this tuple, so it seems prudent to turn it into a struct first to avoid confusion about what the tuple elements mean.
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.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 27, 2025
Rollup merge of rust-lang#136118 - oli-obk:push-qsslxsopnrmr, r=Zalathar Change `collect_and_partition_mono_items` tuple return type to a struct rust-lang#133429 will add a new field to this tuple, so it seems prudent to turn it into a struct first to avoid confusion about what the tuple elements mean.
|
☔ The latest upstream changes (presumably #136135) made this pull request unmergeable. Please resolve the merge conflicts. |
ZuseZ4
force-pushed
the
autodiff-middle
branch
from
9fae9a7 to
c2f90b0
Compare
This comment has been minimized.
This comment has been minimized.
ZuseZ4
force-pushed
the
autodiff-middle
branch
from
c2f90b0 to
d3ed369
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR should not be merged until the rustc_codegen_llvm part is merged.
I will also alter it a little based on what get's shaved off from the cg_llvm PR,
and address some of the feedback I received in the other PR (including cleanups).
I am putting it already up to
Re 1: My test require fat-lto. I also modify the compilation pipeline. So if there are any other llvm-ir tests in the same compilation unit then I will likely break them. Luckily there are two groups who currently have the same fat-lto requirement for their GPU code which I have for my autodiff code and both groups have some plans to enable support for thin-lto. Once either that work pans out, I'll copy it over for this feature. I will also work on not changing the optimization pipeline for functions not differentiated, but that will require some thoughts and engineering, so I think it would be good to be able to run the autodiff tests isolated from the rest for now. Can you guide me here please?
For context, here are some of my tests in the samples folder: https://github.com/EnzymeAD/rustbook
Re 2: This is a pretty serious issue, since it effectively prevents publishing libraries making use of autodiff: EnzymeAD#173. For some reason my dummy code persists till the end, so the code which calls autodiff, deletes the dummy, and inserts the code to compute the derivative never gets executed. To me it looks like the rustc_autodiff attribute just get's dropped, but I don't know WHY? Any help would be super appreciated, as rustc queries look a bit voodoo to me.
Tracking:
r? @jieyouxu