Repository navigation
Support non-differentiable discrete compound assignment operators in ReverseModeVisitor - #2
Open
Elvand-Lie wants to merge 206 commits into
Open
Elvand-Lie wants to merge 206 commits into
Elvand-Lie wants to merge 206 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| addToCurrentBlock(assign_zero, direction::reverse); | ||
| } | ||
| Rdiff = Visit(R, static_cast<Expr*>(nullptr)); | ||
| BinaryOperatorKind valOp; |
There was a problem hiding this comment.
warning: variable 'valOp' is not initialized [cppcoreguidelines-init-variables]
tr));
^| valOp = BO_Shr; | ||
| break; | ||
| default: | ||
| llvm_unreachable("unexpected discrete assignment opCode"); |
There was a problem hiding this comment.
warning: no header providing "llvm_unreachable" is directly included [misc-include-cleaner]
lib/Differentiator/ReverseModeVisitor.cpp:68:
- #include <memory>
+ #include <llvm/Support/ErrorHandling.h>
+ #include <memory>
HessianModeVisitor::Derive walked the parameters once and did two unrelated things per parameter: it accumulated the row length of the hessian matrix and diagnosed an array parameter that came without an index interval, and it derived that parameter's second-order function. The two are interleaved, so the shape of the result is not known until the last derivative has been built, and a request that is going to be rejected still pays for the derivatives of every parameter ahead of the bad one. Split the walk in two: one loop computes IndependentArgsSize and TotalIndependentArgsSize and emits the missing-index-interval diagnostic, and a second loop derives. Nothing else changes -- the derivation loop visits the same parameters in the same order and Merge receives the same three values. The one observable difference is on the error path: clad::hessian(f, "a, b") with an array b no longer emits the second-order function for a before rejecting the call. The diagnostic and the exit code are unchanged, which test/Hessian/ArrayErrors.C pins. The NOLINTNEXTLINE on the do-while is not new debt: the loop is moved verbatim, and moving it puts the line in the diff where clang-tidy's cppcoreguidelines-avoid-do-while would report it.
Elvand-Lie
force-pushed
the
fix/reverse-mode-discrete-compound-assign
branch
from
August 20, 2026 15:09
00e6cb2 to
e438333
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/reverse-mode-discrete-compound-assign
branch
from
August 20, 2026 16:12
e438333 to
dddad62
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/reverse-mode-discrete-compound-assign
branch
from
August 20, 2026 16:30
dddad62 to
832e4fa
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/reverse-mode-discrete-compound-assign
branch
from
August 20, 2026 16:41
832e4fa to
3014a5c
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/reverse-mode-discrete-compound-assign
branch
from
August 20, 2026 17:14
5569902 to
cc3eaaf
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/reverse-mode-discrete-compound-assign
branch
from
August 21, 2026 07:42
cc3eaaf to
9e1b524
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/reverse-mode-discrete-compound-assign
branch
5 times, most recently
from
August 21, 2026 10:15
0f0d489 to
0ee78f8
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/reverse-mode-discrete-compound-assign
branch
from
August 21, 2026 12:29
0ee78f8 to
16aeeb6
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
The pin has not moved since the backend landed: v0.0.224 is from November 2025 and sixty-six releases back, so every fix Enzyme has made since -- and every one clad might report -- is against code nobody here is running. v0.0.290 keeps everything clad depends on. It still exports registerEnzyme(PassBuilder&), which is the whole of clad's link-time surface besides the __enzyme_autodiff calls it emits by name; ENZYME_CLANG and ENZYME_STATIC_LIB are still the options that select the static library, and EnzymeStatic-<LLVM major> is still the target that builds it. Enzyme's own CI covers LLVM 15 through 21, which contains the 19 to 21 window clad requires for this backend.
|
clang-tidy review says "All clean, LGTM! 👍" |
Hessian mode derived the function once in forward mode per requested
direction and reverse-differentiated each of those, so the emitted code grew
as the square of the parameter count: 242k lines of IR for a straight-line
64-parameter function.
The direction does not have to be a compile-time constant. One pushforward
takes it as a run-time argument, and one pullback of that pushforward,
seeded with `{.value = 0, .pushforward = 1}`, differentiates the directional
derivative -- row i of the hessian when the tangent is e_i. That pair serves
every direction, and the wrapper only seeds a tangent, calls, and clears it
again; the same function now emits 14k lines. The adjoints are the rows
themselves, so the pullback accumulates into the matrix with no copy.
The planner decides the scheme and records it on the DiffRequest, which
HessianModeVisitor consumes rather than deciding again, so the two cannot
disagree about which derivatives exist. It also schedules the pushforward,
because a pullback of a function whose body is not built yet comes out
empty. canUseHessianVectorProducts declines what the scheme cannot serve --
it needs one tangent per parameter, passed by position and seedable by
assignment, so no instance methods (the matrix has no place for `this`), no
parameters the pushforward's signature filter skips, and no reference
tangents. Three more cases divert to the per-direction scheme: a custom
forward derivative and a custom pushforward, which only that scheme honors,
and a non-const pointer parameter outside the requested set, which it
diagnoses at compile time instead of dereferencing a null tangent at run
time. hessian_diagonal stays per direction as well.
Two supporting fixes. LookupCustomDerivativeDecl stopped anchoring its
namespace lookup on the CallContext's callee, without which the
custom-pushforward guard never fired: the synthesized request carries the
clad::hessian call, so the lookup searched clad::custom_derivatives::clad::.
And a pullback restricted to a subset of the parameters now carries the
requested indices in its name, f_pushforward_0_pullback, as restricted grads
and hessians do; sharing the plain name emitted two bodies under one
signature and left the linker to keep one of them for both. Checkpointing
pragmas are forwarded into the pullback request the way the per-direction
scheme forwards them into its reverse requests.
On RooFit's 81-parameter HistFactory likelihood this is 30x on hessian
generation (64.8s -> 2.2s), with identical minimizer evaluation counts and
minima at every model size.
Expectations are regenerated from the emitted derivatives. VectorProducts.C
pins the scheme and the subset naming; PerDirectionFallback.C pins the
cases that divert back, next to a control in the same TU that takes the new
scheme -- so the file fails without the change and the diverts are shown to
be per-function. Verified with the full lit suite on release and debug
(live AST-integrity asserts).
The Valgrind row takes two hours where the next slowest takes 47, so it is what every pull request ends up waiting on -- and it is the row least suited to that. It re-runs the whole suite looking for what a change did to memory, which is a property of master rather than something the review would have caught. Keeping it pre-merge without paying that toll would take a merge queue, and those are offered only to organization-owned repositories. Move it to its own job on push. A gate that reports after the fact is only a gate if someone reads it, so a failure files an issue naming the commit and its author, and further failures comment on the open report rather than file another: a week of red master should be one thread, not seven issues.
Forward mode worked out for itself whether a call could contribute to the derivative: VisitCallExpr folded the tangents it had just built and, when they were all zero, emitted the plain call with a zero derivative. Two FIXMEs at that site asked for the decision to move to the planner. The planner now runs varied analysis for forward-mode requests, seeded from the direction the request differentiates along, and VariedAnalyzer records for each call whether any argument is varied. DiffRequest::shouldHavePushforward answers the question the visitor used to answer itself; a call the analysis never saw answers "differentiate it". Only forward-mode requests record and consume this: the requests -enable-va analyzes are unseeded or seeded for a caller, and their verdicts would zero out direction-agnostic pushforwards. A body without a plain call skips the analysis, and a hessian analyzes each parameter once, not once per array index. The record is monotone within a run, so later passes over a loop body can only add varied calls, never lose them. Activity does not care whether a tangent is a const-qualified zero, so the elision now fires for plain parameters too; four tests lose the pushforward of the direction they do not seed. The tangent fold stays as an index-level refinement: a direction seeding p[1] leaves the whole of p varied, and only the tangents know f(p[0]) is inactive. A "not varied" verdict now deletes derivatives by default, so the analyzer is made conservative where it does not track state: a DeclRefExpr naming a function (an assert, reachable on master with -enable-va), a call through a function pointer (a crash on a null getDirectCallee()), and member state reached through `this`, which is not tracked per field and can carry the derivative -- this also covers functors differentiated with respect to a field, whose FieldDecl the seed set cannot hold. ActivityForward.cpp pins these with and without -enable-va, which used to run the forward analysis unseeded; ZeroTangentCalls.C pins the once-per-parameter array hessian. CI surfaced two more places where the verdict was wrong. An object built from varied constructor arguments must read as varied when handed to a call, but a reference field's VarData held an empty dependency set -- the analysis does not model constructor bodies, so nothing ever fills it -- and setIsRequired silently dropped the bit there: a Kokkos functor capturing the independent variable by reference lost its parallel_for and the derivative came out zero. A reference field now keeps one bit of its own. And the marks this pre-pass records must feed only shouldHavePushforward: DiffRequest::isVaried consulted them unconditionally, and since an argument handed to a non-const pointer parameter is marked varied no matter what it is, a CUDA kernel launch's config call -- literal dimensions and a null stream -- stopped counting as all-literal and the planner scheduled a derivative for __cudaPushCallConfiguration, warning about its missing definition. isVaried reads the marks only under -enable-va, as before the pre-pass existed. ActivityForward.cpp pins the reference capture, FunctionCalls.C the all-literal call, and the zero tangents in BuiltinDerivatives.C spell their float suffix per platform.
The wrapper mallocs the object and its adjoint, fills them in, and returns
`{*_this, *_d_this}` -- copies, with the two blocks left behind. Every call
leaks them, twice sizeof(T), for the lifetime of the program. BuildThisExpr
already builds the matching free() and hands it back; the pullback path
collects it, this one dropped it, and the adjoint object never had one at all.
Copy the result into a local before releasing them, since returning the list
directly leaves nowhere for the free()s to go. Valgrind over the test suite
reported 38 leak records before this and 18 after, all of the remainder from
allocations the tests themselves never free.
Five tests allocate and never release: Constructors.C builds two arrays in main, MemberFunctions.C and Casts.C `new` an object they read once, Pointer.C `new`s two pointers and an array, and FunctionCalls.C hands its `int*` to a destroy() that only runs the destructor. The derivative inherits each one, since clad reproduces the allocation and adds an adjoint beside it, so a leak check over the generated programs reports every one of them twice. Release them where the primal is done with the storage, holding the value in a local first where the result is read through the pointer being freed. clad differentiates delete -- VisitCXXDeleteExpr releases the adjoint alongside the primal -- so the derivatives free both, which the expectations here now record.
Every tape carried a std::mutex, whether or not it was the multithreaded one: 64 bytes on macOS, a constructor and a destructor call per tape object, and a generated pullback keeps hundreds of tape objects in its frame. Only the multithreaded push and pop ever lock it. The mutex now lives in a base the multithreaded instantiation alone brings in, so a single-threaded tape of doubles shrinks from 632 bytes to 568 and has nothing to destroy. The lock's accessor moves with it, and its callers, the multithreaded overloads, are unchanged.
`#pragma clad checkpoint loop` makes the reverse sweep recompute each iteration's body instead of restoring it from a tape, and it recomputes from the state the previous reverse iteration left, not from what the forward iteration saw. A body that reads a variable it wrote in the iteration before, `t = t * x`, or a product it accumulates, `p *= x`, therefore recomputes from the wrong value, and the gradient is wrong with nothing to say so: 88 where 17 is right, on a three-iteration loop. What a loop hands from one iteration to the next is a fact about the loop, so the loop analysis proves it and the reverse-mode visitor refuses the pragma with an error at the loop and a note at the read. A variable the body only adds to or subtracts from is exempt, since the adjoint of `s += e` never reads s; one assigned at the top of the body, or declared in it, is the iteration's own; the index is stepped by the loop and restored by the reverse. An assignment below the top level does not count as assigning, since a path may skip it, so the check errs on the side of refusing. Proving it costs a walk of every loop body, and only the pragma asks, so a request that carries none does not pay for it: on the ATLAS likelihood in rootbench, 147 loops per channel, the derivation stays at 15.8 s.
The early-return encoding splits the forward sweep and splices the reverse one, and each walk had a branch for the case where the sweep is not a compound statement. Neither can run: a function body is a compound statement ([dcl.fct.def.general]), so what the visitor makes of it is compound too, and the one thing that could be null is filtered before the loop. Both branches were reported as uncovered on vgvassilev#2218. Drop them, and say the invariant with a cast rather than a test, so an assertions build says so if it is ever broken.
By-value construction from named class objects reused a DeclRefExpr across member-init assignments and the adjoint constructor call, which fails the shared-AST integrity check.
The lookup also searched the default paths, PATH included. With a clang on PATH its LLDConfig.cmake loads the compiler's LLVMConfig.cmake over the one clad found; for ROOT built with clang 22 that sets LLVM_LINK_LLVM_DYLIB and clad-tblgen fails to link -lLLVM. Search only LLD_DIR and next to the found LLVM, with NO_DEFAULT_PATH, as LLVM_DIR is searched.
Bumps [urllib3](https://github.com/urllib3/urllib3) from 2.7.0 to 2.8.0. - [Release notes](https://github.com/urllib3/urllib3/releases) - [Changelog](https://github.com/urllib3/urllib3/blob/main/CHANGES.rst) - [Commits](urllib3/urllib3@2.7.0...2.8.0) --- updated-dependencies: - dependency-name: urllib3 dependency-version: 2.8.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
In reverse mode, Clad tried to look up custom derivatives for calls marked as non-differentiable. This caused errors for integer arguments because built-in pushforwards expect floating-point types. For example: double result = 0.5 * std::log(2); Clad tried to find a pushforward for std::log<int>, which failed to match the standard log_pushforward<T> and caused compilation to fail. Since non-differentiable calls have zero derivatives, the pushforward is not needed. This patch skips the lookup for such calls and avoids the spurious compilation error.
Currently, Clad only generated adjoints for conditional operators that were modifiable lvalues. For pointer-typed conditionals, the adjoint pointer was left uninitialized. This led to a crash in the reverse sweep. This patch generates adjoints for pointer-typed conditionals using the adjoints of their branches.
The fallback for an unsupported statement reports it and then clones it, so
the derivative keeps the statement and the compile finishes -- what throw,
labels and goto already do. Four kinds had no clone to reach: StmtClone's
fallback asserts and returns null, and with NDEBUG only the null survives, so
ReferencesUpdater walked it and the compiler died on a statement clad had just
finished saying it would leave alone.
x ?: 1.0 BinaryConditionalOperator
if (c) [[likely]] { ... } AttributedStmt
P p(x, 2.0); CXXParenListInitExpr
struct D : B { using B::B; } CXXInheritedCtorInitExpr
Give the four a clone each. Three are transcriptions of their constructors;
`a ?: b` is not, because it evaluates `a` once and names it with an
OpaqueValueExpr that the condition and the true branch both point at. Cloning
those sub-expressions on their own would leave them pointing into the original,
so bind the clone of the opaque value first and let VisitOpaqueValueExpr
substitute it, the way VisitPseudoObjectExpr already does for the same problem.
The common sub-expression and the opaque value's source are one node, and
BinaryConditionalOperator's constructor asserts that they are, so the clone is
made once and shared rather than cloned twice.
Stop the fallback handing back a null at all. A kind with no case has no clone
to give, so it now reports that clad cannot differentiate the function and
leaves a placeholder so the tree stays well formed until the error ends the
compilation. That covers the kinds still without a case -- CXXFoldExpr,
GenericSelectionExpr, AtomicExpr, CXXNoexceptExpr, GCCAsmStmt -- rather than
only the four above.
CXXParenListInitExpr arrived in clang 16 (P0960R3), so its clone and its test
are behind that version. Its union form carries the initialised member in the
slot the array filler occupies, so the clone carries whichever of the two is
set. An array initialised this way is refused and said to be refused: clad's
array handling recognises InitListExpr and not this node, so even a faithful
clone reaches Sema as an element initialiser carrying the whole array's type.
An unsupported expression directly in a return statement still fails to
compile, now saying so rather than crashing: the derivative has no tangent to
return, which is what being unsupported means. `return x ?: 1.0;` reports that
it is unsupported and then that the derivative returns nothing, where before it
reported the first and segfaulted on the second.
Fixes: vgvassilev#2088
The lookup also searched the default paths, PATH included. With a clang on PATH its LLDConfig.cmake loads the compiler's LLVMConfig.cmake over the one clad found; for ROOT built with clang 22 that sets LLVM_LINK_LLVM_DYLIB and clad-tblgen fails to link -lLLVM. Search only LLD_DIR and next to the found LLVM, with NO_DEFAULT_PATH, as LLVM_DIR is searched.
DefaultArgumentAdjoint.C checked its gradients by comparing them in main and turning a mismatch into a non-zero exit, so a failure reported only that the binary exited 1 -- not which of the thirteen derivatives was wrong, nor what it came out as. Print each one and match it with CHECK-EXEC, as the other execution tests do, so a failure names the derivative and shows both values.
issue-2088.cpp exercises the cloning of statement kinds clad diagnoses as unsupported, and its AttributedStmt case used [[likely]]. This change differentiates through likelihood hints, so that statement no longer draws the warning and -verify failed on every row. Wrap the statement in [[clang::nomerge]] instead, which is still not supported and so still cloned; it needs a call to apply to.
Clad's Jacobian API generates a void-returning derivative and exposes differentiable outputs through pointer, reference, or array parameters. Reject non-void primal functions early in DiffPlanner during top-level request validation (after target function resolution and mode selection, before request graph scheduling, analyses, or custom derivative lookup). This ensures matching custom Jacobian derivatives cannot bypass the strict void-returning contract. Emit a clear error at the differentiation request site and a companion note identifying the primal declaration and return type, safely falling back to the function location for trailing-return syntax. Add regression coverage for scalar returns, output pointers with scalar returns (reproducing Issue vgvassilev#1306), status-returning functions, matching custom derivatives, and trailing-return functions. Fixes vgvassilev#1306 Signed-off-by: Tempris Admin <elvandlie@gmail.com>
`%=`, `&=`, `|=`, `^=`, `<<=` and `>>=` reached the llvm_unreachable at the end of the assignment branch. They act on integers and their result is piecewise constant in both operands, so they take the path `=` takes with nothing passed on: the adjoint of the overwritten value is zeroed, and the right side is visited only for its side effects. The left side is stored and restored like any other assignment's, and no `_r_d` copy of its adjoint is made, since nothing reads it. The value the reverse sweep reads for the assignment is the left side itself, which still holds the result there. Left unset, it would fall back to the forward expression and assign a second time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Elvand-Lie
force-pushed
the
fix/reverse-mode-discrete-compound-assign
branch
from
October 12, 2026 12:43
a6977a9 to
6a6721d
Compare
For x[i++] and x[i--], the reverse pass restored i and then evaluated the index again when accessing the adjoint array. This caused the restore and update to cancel out, so all contributions were applied to the same element. Use the restored index value instead of evaluating it again.
…ilev#2057) * [VA] Preserve accumulated state and report lattice merge changes Make AnalysisBase merges report newly introduced stable state while preserving locally accumulated values during ancestor reconciliation. VariedAnalyzer uses the returned change flag to drive re-analysis. Unreported stable growth can therefore stop its worklist before reaching a fixpoint. TBRAnalyzer ignores the flag but consumes the merged state, so preserving local state remains relevant to both consumers. Materialize missing array elements from the target's conservative default element before recursively merging them. Preserve local entries while reconciling inherited state, and report only stable merge growth. Do not report the final ancestor refresh because block traversal may immediately clear that transient state and cause worklist oscillation. Add a lit regression exercising loop-carried activity through the generated derivative instead of testing AnalysisBase implementation details directly. * [NFC][UA] Rebase UsefulAnalyzer onto the AnalysisBase lattice Replace UsefulAnalyzer's duplicated state storage and traversal machinery with the copy-on-write VarsData lattice from AnalysisBase. Link blocks through first-discovery m_Prev chains. Direct continuations are reprocessed when their parent is visited; other edges are re-enqueued only when merging introduces a previously unseen variable. Represent whole-variable usefulness through scalar presence markers, independent of field, pointee, or declaration types. UsefulAnalyzer queries key presence rather than payload values, and its scalar payload bits remain false. This port depends on PR vgvassilev#2058 reporting insertion of a previously unseen variable as stable merge growth. The array-default and payload-shadowing semantics in that prerequisite do not affect UsefulAnalyzer's presence-only lattice. This is intended to be a no-functional-change port. Existing lit coverage in UsefulForward.cpp, UsefulNonTermination.cpp, and the merged vgvassilev#2060 UsefulLattice.cpp exercises generated-code behavior. Depends on: vgvassilev#2058
…ev#2247) A counted loop's trip count and its counter were declared with the target's canonical size_t -- unsigned long on 64-bit targets, unsigned int on the 32-bit row -- whatever the loop stepped through. The type was made up, the generated code differed per target, and a loop over std::size_t still got a counter spelled unsigned long. The induction variable steps once per iteration within its own type, so the count fits that type's unsigned counterpart: the type itself when it is unsigned. Take that, spelled as the source spells it wherever the derivative can name the spelling: utils::getSpellableType desugars one step at a time until it reaches a spelling that is not decltype, not local to another function and accessible where the derivative lives, after CppInterOp's getWrapperSpellableType. The counter over an int loop is then unsigned int on every target, and over std::size_t, std::size_t.
Add six microbenchmarks measuring allocation headroom for activity-analysis changes: a positive control, interprocedural passive work, a dead struct field, a dead array chain, branch-guarded work, and useful-but-not-varied values. Each case provides a baseline for a different kind of reduction. Keep the kernel definitions in benchmark/ActivityWasteKernels.h so the benchmark and its lit correctness test exercise the same functions. The test checks analytic gradients at -O0 and -O3 -DNDEBUG, including both branch paths, zero derivative components, repeated calls with reset state, and the scratch-array upper bound. Numerical checks remain active when NDEBUG is defined. Reset the external sink before each affected benchmark invocation and observe it after the call. Allocate input and derivative buffers outside the counted region. Snapshot allocation totals before publishing counters, so allocations made by counter-map insertion do not contaminate AllocBytes. Measured tape-block counts on the local 64-bit LLVM/Clang 18.1.8 build: | Case | Range | Tape blocks | Reduction target | |-------------------|-------------------|-------------|---------------------------| | positive control | n 64 -> 4096 | 0 -> 4 | allocation control | | interproc passive | work 100 -> 5000 | 50 -> 250 | interprocedural summaries | | dead struct field | n 64 -> 4096 | 1 -> 12 | field-sensitive activity | | dead array chain | n 1000 -> 10000 | 1 -> 10 | array-aware pruning | | branch-guarded | n 64 -> 4096 | 0 -> 4 | usefulness analysis | | useful-not-varied | reps 100 -> 10000 | 3 -> 30 | variedness analysis | Reproduce with: ./benchmark/ActivityWaste --benchmark_counters_tabular=true For this build the measured blocks are 8224 bytes. Block counts change in steps, not continuously with the range parameter: the interprocedural case uses 50 blocks for work 100, 125 and 625, then 150 at 3125. The n=64 positive-control and branch-guarded cases allocate no tape blocks, so that size is not an allocation-counter control. These are build-specific baselines, not portable byte-size guarantees or timing claims. The harness follows benchmark/MemoryComplexity.cpp, with floating-point per-iteration division and snapshot-before-publication accounting. That existing benchmark is unchanged; sharing its instrumentation is outside this PR. Related to issue vgvassilev#1873
A function that returns void leaves its result in a parameter, and until now
the forward derivative computed the tangent of that parameter into a local and
stopped there. The derivative was right and unreachable: the signature was the
primal's, so there was nowhere to put it and nothing the caller could read.
void fn(double i, double j, double k, double& res) { res = i * j * k; }
auto d = clad::differentiate(fn, "i");
double res = 0;
d.execute(2, 3, 4, res); // res == 24, and dres/di went nowhere
Give such a derivative the tangent's type as its return type and return it, so
the value the pushforward already knows how to compute reaches the caller
without the wrapper that was the only way to ask for it.
The signature is decided in two places that have to agree. GetDerivativeType
returned the primal's own type for forward mode; it now returns the tangent of
the sole output parameter when the primal returns void.
ExtractDerivedFnTraitsForwMode has to say the same thing from the header side,
or CladFunction::execute calls a function whose return type it has wrong. It
is written recursively rather than as a fold expression because part of the
test suite compiles these headers as C++14. Which parameter it is, is a fact
about the primal, so a DiffRequest answers it -- getOutputTangentParam -- and
the visitor reads it rather than working it out again.
An output parameter here is a non-const lvalue reference to a floating point
type. Pointers are left out: how many elements one addresses is not something
the signature says. An `int&` a function counts through is not one either, nor
a reference to an enum, which has no tangent for the derivative to find.
Member functions are left out as well -- the traits describe their derivative
through the primal's member pointer type, which has no room for a changed
return type, and the two disagreeing is worse than the wrapper. A static
member function is addressed as a plain function pointer, so it is in scope.
More than one output parameter is an error rather than a guess. Forward mode
hands back one tangent, so any choice among several outputs is arbitrary, and
the wrapper still expresses which one is wanted. DiffCollector reports it,
where the differentiation was asked for, so the error points at the
clad::differentiate call rather than at a function the user may not own.
A function with no output parameter keeps its void return and is unaffected.
A custom `_darg` derivative is matched on its whole signature, return type
included, so one written for a void primal returns the tangent too. A `void`
one is not quietly dropped in favour of clad's own: clad reports that a
user-defined derivative was provided but not used and names the signature it
expected. CustomDerivatives.rst says so, quoting
test/Documentation/CustomDerivatives/OutputParameter.cpp, which runs.
The operands of a sum share an adjoint seed. If that seed contains a tape read, cloning the AST node for each operand duplicates the pop operation. For example, when reversing (x + y) * sin(y), cloning the seed generated: _d_x += clad::pop(_tape); _d_y += clad::pop(_tape); // BUG: Pops the tape a second time This leads to wrong results and this patch fixes the issue by evaluating side-effecting seeds into a temporary variable exactly once (e.g., tmp = pop()) and distributing that temporary to the operands.
clad declares a temporary for the index argument a pullback writes back to, and names it after the container's nested type. Printed on its own, `size_type` is not a name anything can look up, so the declaration has to carry the container with it. The test differentiates through a const vector, which is what reaches the pullback and so declares such a temporary.
A reverse_forw function has no reverse sweep, so pushing primals onto the tape is unnecessary. Store the value in a local variable instead.
Register a scheduled derivative against its own known prototype, including specializations whose unrelated instantiations must remain separate. Reserve pushforward tangent parameter names in scope and avoid primal parameter collisions during repeated differentiation. Add generated-body and runtime checks for third and fourth derivatives of sin, third derivatives of exp, and sin nested in arithmetic. Fixes: vgvassilev#2081
rsqrtf, __logf and __expf are all unary, and a call to a unary function resolves to a pushforward in both directions: the pullback would only multiply by f'(a), which is the pushforward's second half already. What BuiltinDerivativesCUDA.cuh declared was the pullback alone, with no pushforward beside it, so nothing could reach the three and reaching one was an error rather than a fallback -- reported against the caller's own line, and calling a derivative clad ships itself "user-defined". Fixes: vgvassilev#2172
func14 came in with d40fc7f while vgvassilev#2201 was in review, so vgvassilev#2201 never saw it. Since vgvassilev#2201 a seed that pops a tape is stored in an `_r` temporary rather than cloned, and with both on master func14's CHECK lines still expect the inlined pop, failing every build. The generated gradient is correct; its CHECK-EXEC line passes unchanged. Fixes vgvassilev#2250, Fixes vgvassilev#2251.
numerical_diff deduces the target function type from its argument. Clad passes a reference to the overload or specialization the original call resolved to, so deduction succeeds in the AST, but the printed derivative names only the function, e.g. `forward_central_difference(std::tgamma, ...)`, which does not compile for an overload set or a function template. Write the type as the first template argument, as one would in source: `forward_central_difference<double (*)(double)>(...)`. Clad calls the same instantiation; the printed code now selects it too. Lookups now become expressions through one helper, utils::BuildDeclarationNameExpr, which also builds the template-id and asserts its arguments carry the location clang needs to keep them. The type is spelled through getSpellableType, which gains what a function type needs: the parts of pointer, reference and function types are spelled as well, and a name printed unqualified must be declared in a scope enclosing the code, so `Real` declared in namespace `M` becomes `double` outside of it. Replaces the cast proposed in vgvassilev#340.
This file contains hidden or 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
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 resolves an ICE in
ReverseModeVisitorwhen encountering discrete compound assignment operators (%=,&=,|=,^=,<<=,>>=).Summary of Changes
Reachable ICE Resolution & Explicit Opcode Handling:
BO_RemAssign,BO_AndAssign,BO_OrAssign,BO_XorAssign,BO_ShlAssign, andBO_ShrAssign.BO_Rem,BO_And,BO_Or,BO_Xor,BO_Shl,BO_Shr), with unhandled opcodes terminating atllvm_unreachable.Discrete Derivative-Boundary Semantics:
_d_L = 0;) exactly once. Continuous adjoints do not propagate across discrete boundaries to RHS.Nested Compound-Expression Preservation & Side-Effect Evaluation:
(L_old op R)value expressions forvalueForRevPass.next(y)), stores the evaluation viaStoreAndRefin the forward pass and updates bothRdiff.updateStmtandRdiff.updateRevSweepso that RHS side-effectful expressions evaluate exactly once across both forward assignment and reverse value calculation.Regression Tests:
test/Gradient/CompoundBitwiseOps.Ccovering statement-level assignments, nested expressions, loop iterations, FileCheck AST validation, and runtime execution assertions.