Support discrete compound assignments in reverse mode - #2000
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
5569902 to
cc3eaaf
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cc3eaaf to
9e1b524
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
9e1b524 to
ef391da
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
d1a541e to
0f0d489
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
0f0d489 to
0ee78f8
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
ReverseModeVisitor entered its assignment path for all Clang assignment operators but handled only =, +=, -=, *=, and /=. Remainder, bitwise, and shift compound assignments fell through to an llvm_unreachable. Add explicit handling for %=, &=, |=, ^=, <<=, and >>= as discrete derivative boundaries. Preserve the primal compound assignment result as a forward lvalue-reference declaration (_ref) while snapshotting its converted post-assignment value (_t) for the reverse sweep. Use a primal result override to return the materialized forward reference directly, eliminating binary operator reconstruction that previously caused double execution and shared AST node assertions. Restructure old-value adjoint storage to avoid emitting unused declarations. Add generated-code and runtime regression coverage for all six operators, nested assignments under parent compound assignment, narrowing conversions, loops, and side-effectful RHS expressions. Signed-off-by: Tempris Admin <elvandlie@gmail.com>
0ee78f8 to
16aeeb6
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
| } | ||
|
|
||
| // CHECK-LABEL: f_and_grad | ||
| // CHECK: int &_ref0 = n &= 7; |
There was a problem hiding this comment.
Can we make sure we check all the bodies with CHECK-NEXT? Also the tests should be tight (almost surgical).
| Expr* assign_zero = BuildOp(BO_Assign, CloneNode(ResultRef), zero); | ||
| addToCurrentBlock(assign_zero, direction::reverse); | ||
| } | ||
| Rdiff = Visit(R, static_cast<Expr*>(nullptr)); |
There was a problem hiding this comment.
| Rdiff = Visit(R, static_cast<Expr*>(nullptr)); | |
| Rdiff = Visit(R); |
| } else if (opCode == BO_RemAssign || opCode == BO_AndAssign || | ||
| opCode == BO_OrAssign || opCode == BO_XorAssign || | ||
| opCode == BO_ShlAssign || opCode == BO_ShrAssign) { |
There was a problem hiding this comment.
| } else if (opCode == BO_RemAssign || opCode == BO_AndAssign || | |
| opCode == BO_OrAssign || opCode == BO_XorAssign || | |
| opCode == BO_ShlAssign || opCode == BO_ShrAssign) { | |
| } else if (isDiscreteAssign) { |
| else if (!isPointerOp) | ||
| oldValue = StoreAndRef(CloneNode(ResultRef), direction::reverse, "_r_d", | ||
| /*forceDeclCreation=*/true); | ||
| if (!isDiscreteAssign) { |
There was a problem hiding this comment.
If !isDiscreteAssign do we really need to pass through the other code?
| utils::getNonConstType(Ldiff.getExpr()->getType(), m_Sema); | ||
| QualType referenceType = m_Context.getLValueReferenceType(valueType); | ||
| Expr* forwardResult = | ||
| StoreAndRef(assignExpr, referenceType, direction::forward, "_ref", |
There was a problem hiding this comment.
Binding a reference to the result of the assignment does not work when the left side is a bit-field, and the user sees a compiler error in code they did not write.
int &_ref0 = n %= y; is fine for an ordinary variable. For s.flags &= mask; where flags is a bit-field, it is ill-formed: a non-const lvalue reference cannot bind to a bit-field ([class.bit]/5). The generated code will not compile, and the error will point into clad's output rather than at the line the user wrote.
Bit-fields are not common in code people differentiate, so this may be worth no more than a guard that falls back to the old path when ResultRef refers to one, which FieldDecl::isBitField() answers.
| } else if (opCode == BO_RemAssign || opCode == BO_AndAssign || | ||
| opCode == BO_OrAssign || opCode == BO_XorAssign || | ||
| opCode == BO_ShlAssign || opCode == BO_ShrAssign) { | ||
| if (ResultRef && !isPointerOp) { |
There was a problem hiding this comment.
This decides whether the adjoint gets zeroed, and only one side of it is ever taken by a test.
Every function in the new test assigns to a plain local int: n %= y, n &= 7, mask %= 4. None uses an array element, a pointer dereference, or a struct member, so isPointerOp is false every time and the zeroing always happens. The path where it is skipped is not exercised at all.
That matters because skipping the zero is what would leave a stale adjoint behind, and a stale adjoint gives a wrong gradient rather than a crash. A function doing arr[i] %= y and then reading arr[i], with a CHECK-EXEC on the result, would say whether the skip is right.
| StoreAndRef(assignExpr, referenceType, direction::forward, "_ref", | ||
| /*forceDeclCreation=*/true); | ||
| valueForRevPass = | ||
| GlobalStoreAndRef(CloneNode(forwardResult), valueType, "_t", |
There was a problem hiding this comment.
Is this stored value ever read for a discrete operation?
The adjoint of the left side is set to zero rather than propagated, and the right side is visited with a null adjoint, so as far as I can tell nothing in the reverse sweep needs the post-assignment value. If that is right, force=true inside a loop pushes one value per iteration onto the tape and never pops it, which costs memory proportional to the trip count for nothing.
I could not confirm this either way from the diff, since the test's CHECK lines do not cover the generated loop body. If the value is needed, a sentence saying what reads it would help; if it is not, dropping the store would make the branch smaller.
2 participants
Description
ReverseModeVisitor entered its assignment path for all Clang assignment operators but explicitly handled only
=,+=,-=,*=, and/=. Remainder, bitwise, and shift compound assignments (%=,&=,|=,^=,<<=,>>=) fell through to anllvm_unreachable.This PR adds discrete compound assignment support in reverse mode:
(L op= R)is evaluated exactly once in the forward pass inside an lvalue-reference declarationT& _ref = (L op= R);. A primal result override returns this reference directly as the forward expression result, preserving C++ lvalue semantics for nested operations.T _t = _ref;(orclad::tape<T>inside loops) usingGlobalStoreAndReffor use in the reverse sweep.oldValueadjoint storage to avoid emitting unused_r_ddeclarations for discrete operations.test/Gradient/CompoundBitwiseOps.Ccovering all six operators, nested assignments under parent*=, narrowing conversions, loops, and side-effectful RHS expressions.