Fix autodiff empty ret regression #147200
Conversation
rustbot
added
A-LLVM
This comment has been minimized.
This comment has been minimized.
ZuseZ4
force-pushed
the
fix-autodiff-emptry-ret
branch
from
1791078 to
4125b9b
Compare
This comment has been minimized.
This comment has been minimized.
ZuseZ4
force-pushed
the
fix-autodiff-emptry-ret
branch
from
4125b9b to
8b8cbe0
Compare
| let fn_ret_ty = builder.cx.val_ty(call); | ||
| if fn_ret_ty != builder.cx.type_void() && fn_ret_ty != builder.cx.type_struct(&[], false) { | ||
| builder.store_to_place(call, dest.val); | ||
| } |
There was a problem hiding this comment.
An equality test against an empty struct type seems unlikely to be the right check here, but I don't know enough about LLVM IR to say what the right check would be.
Also, is there any reasonable way to test the non-void case?
There was a problem hiding this comment.
Every other test case in codegen-llvm/autodiff handles the non-void cases, we apparently just never had one for the void case since before the refactor it just used to work.
There was a problem hiding this comment.
Sorry, what I mean is the empty-struct case. Is that covered by tests?
There was a problem hiding this comment.
Ah yes, that's what this test is testing.
// CHECK-NOT: store {} undef, ptr undef the line is currently generated by rustc master since the call (before this PR) returns an empty struct, {}. I haven't added a test for void ret, since we now (since the last refactor) don't seem to generate void returns anymore, so I can't test it. I still have added handling for it, just in case that this changes again in the future.
As an example for what we currently generate. I'm testing for us not generating the store anymore, since it's the part causing UB.
%27 = call {} (...) @__enzyme_autodiff_ZN5piprx8d_energy17hfdc5833849837ebfE(ptr @_ZN5piprx16f_energy_inplace17h5cbf6d64465cb049E, metadata !"enzyme_dup", ptr %18, ptr %20, metadata !"enzyme_const", ptr %22, metadata !"enzyme_dup", ptr %24, ptr %26)
store {} %27, ptr undef, align 1
Other than that, testing for equality against the empty struct {} isn't overly pretty, but at that point we already lost the Rust types and only have 'll types left, so we can't use all the nice Rust functions to check for things. It didn't seem worthwhile to me to carry them around till the end since it feels like too much code just to make the check a little prettier, but I don't have overly strong opinions about it.
There was a problem hiding this comment.
This is not necessarily sufficient, you can have an empty struct of an empty struct (and that is indeed used within enzyme).
There was a problem hiding this comment.
you should look for the c binding of https://llvm.org/doxygen/classllvm_1_1Type.html#a0f5c23d3b941362c2fa195f4c0b99683
There was a problem hiding this comment.
I was wondering about using something more general, but if that's needed, we should likely update our logic elsewhere, where we generate those declarations.
Fyi, the bug here materialized before we ran enzyme. It was because we now generate the enzyme decl (if it doesn't return) as having the {} return type. In my previous code I had generated them to have void ret type, I'd be fine to change it back to void ret. Our logic doesn't generate __enzyme declarations with nested empty return types, so they can't show up here. If you have a better rule on how to generate the return type of enzyme for zst I'm also open to update it.
|
(If you just want to get the void fix rubber-stamped and worry about the finer details later, please add some kind of suitable FIXME and I'll approve.) |
ZuseZ4
force-pushed
the
fix-autodiff-emptry-ret
branch
from
8b8cbe0 to
de189fa
Compare
|
No worries, I'd like to land it but my colleagues are also used to use a custom rustc fork anyway, so there is no rubber-stamping needed if you see a better way of testing/handling it. I added some documentation since it clearly wasn't self-explanatory. Does that help? |
|
It feels like there should be some more general way to ask LLVM “is this a type that makes no sense to store”, but I don't have enough LLVM knowledge to say what that better way would be. At any rate, the check in this PR seems clearly better than the status quo, so I'm fine with merging it. @bors r+ |
bors
added
S-waiting-on-bors
Rollup of 11 pull requests Successful merges: - #146918 (add regression test) - #146980 (simplify setup_constraining_predicates, and note it is potentially cubic) - #147170 (compiletest: Pass around `DirectiveLine` instead of bare strings) - #147180 (add tests) - #147188 (Remove usage of `compiletest-use-stage0-libtest` from CI) - #147189 (Replace `rustc_span::Span` with a stripped down version for librustdoc's highlighter) - #147199 (remove outdated comment in (inner) `InferCtxt`) - #147200 (Fix autodiff empty ret regression) - #147209 (Remove `no-remap-src-base` from tests) - #147213 (Fix broken STD build for ESP-IDF) - #147217 (Don't create a top-level `true` directory when running UI tests) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #147200 - ZuseZ4:fix-autodiff-emptry-ret, r=Zalathar Fix autodiff empty ret regression closes #147144 The two gsoc summer projects caused a bit of churn, which was to be expected, especially since we don't run autodiff in CI yet. This adds a void return testcase that we should have had anyway, and fixes the regression. r? `@Zalathar` (Just guessing since I've seen you in a few LLVM PRs and Oli is probably still busy. Feel free to reroll!)
closes #147144
The two gsoc summer projects caused a bit of churn, which was to be expected, especially since we don't run autodiff in CI yet.
This adds a void return testcase that we should have had anyway, and fixes the regression.
r? @Zalathar (Just guessing since I've seen you in a few LLVM PRs and Oli is probably still busy. Feel free to reroll!)