Fix autodiff debug builds #140030
Fix autodiff debug builds #140030
Conversation
rustbot
added
S-waiting-on-review
[DO NOT MERGE] start building enzyme on x86_64-gnu-llvm-{19|20} builders
My goal is to put this in CI on April 26, to have a week to land some of the outstanding PRs:
rust-lang#139700
rust-lang#139308
rust-lang#139557
rust-lang#140030
rust-lang#140049
The autodiff flags PR should land first, but otherwise they don't overlap and are mostly ready, so it shouldn't be too hard to land them. In the meantime, I'll experiment here with some builders.
r? `@oli-obk`
Tracking:
- rust-lang#124509
try-job: x86_64-gnu-llvm-19
try-job: x86_64-gnu-llvm-20
[DO NOT MERGE] start building enzyme on x86_64-gnu-llvm-{19|20} builders
My goal is to put this in CI on April 26, to have a week to land some of the outstanding PRs:
rust-lang#139700
rust-lang#139308
rust-lang#139557
rust-lang#140030
rust-lang#140049
The autodiff flags PR should land first, but otherwise they don't overlap and are mostly ready, so it shouldn't be too hard to land them. In the meantime, I'll experiment here with some builders.
r? `@oli-obk`
Tracking:
- rust-lang#124509
try-job: dist-x86_64-linux
| // We can't differentiate a function that got inlined. | ||
| let autodiff_active = cfg!(llvm_enzyme) | ||
| && matches!(mono_item, MonoItem::Fn(_)) |
There was a problem hiding this comment.
Discussion: this kinda feels backwards to me, I feel like if autodiff is applied to some fn it should inhibit inlining. Is there a small test case (synthetic or not) that can demonstrate this change? Or rather, under what situation can this condition be previously wrong (the change itself looks reasonable)?
There was a problem hiding this comment.
I add inline(never) in my frontend macro to source and target function. However, those aren't guaranteedon a language level to prevent inlinung, so I additionally check it here in rustc_monomorphize. So yes, autodiff prevents inlining. (Edited a bit, but I think I still miss your point).
https://doc.rust-lang.org/nightly/reference/attributes/codegen.html?highlight=inline
Pretty much every codegen test will fail if you remove these two barriers.
The change btw. is needed since we currently also inspect non fn items like Asm, which triggers a debug assertion, causing bootstrap to fail. But you already mentioned that this looks reasonable to you
There was a problem hiding this comment.
But I feel like I probably misunderstand your question?
I moreso meant if there's a smaller example that would fail without this PR, not changing the front-end/middle-end bits, as a regression test.
... But also not building -> building seems like an improvement, yes 😆
There was a problem hiding this comment.
aah, yes now I get your question. I only mentioned it in the title, not the PR body, but you can find the panic stacktrace in the linked issue.
There was a problem hiding this comment.
@jieyouxu since you already looked at it, it's a one line change, fixes bootstrap and eliminates the ad overhead, do you mind to rubberstamp it? Then I can drop it from my final enable-aitodiff PR.
There was a problem hiding this comment.
Yeah, at least this makes it possible to build, and I can't come up with a smaller test...
|
r? jieyouxu @bors r+ rollup |
bors
added
S-waiting-on-bors
…enton Rollup of 8 pull requests Successful merges: - rust-lang#139946 (fix missing word in comment) - rust-lang#139982 (SystemTime doc tweaks) - rust-lang#140009 (docs(LocalKey<T>): clarify that T's Drop shouldn't panic) - rust-lang#140021 (Don't ICE on pending obligations from deep normalization in a loop) - rust-lang#140029 (Relocate tests in `tests/ui`) - rust-lang#140030 (Fix autodiff debug builds) - rust-lang#140120 (Use `output_base_dir` for `mir_dump_dir`) - rust-lang#140121 (Document why CodeStats::type_sizes is public) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#140030 - EnzymeAD:autodiff-debug, r=jieyouxu Fix autodiff debug builds r? `@oli-obk` closes: rust-lang#139704 Tracking: - rust-lang#124509
r? @oli-obk
closes: #139704
Tracking: