Repository navigation
Diagnose non-void functions in Jacobian mode - #3
Open
Elvand-Lie wants to merge 183 commits into
Open
Elvand-Lie wants to merge 183 commits into
Elvand-Lie wants to merge 183 commits into
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/jacobian-void-return-scalar-fn
branch
from
August 20, 2026 15:03
a5baa12 to
ee64a89
Compare
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.
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/jacobian-void-return-scalar-fn
branch
from
August 20, 2026 17:17
81f3d63 to
a6d8847
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/jacobian-void-return-scalar-fn
branch
from
August 21, 2026 08:44
a6d8847 to
870f7ba
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return true; | ||
| return false; | ||
| } | ||
| static bool hasAttribute(const Decl* D, attr::Kind Kind) { |
There was a problem hiding this comment.
warning: no header providing "clang::attr::Kind" is directly included [misc-include-cleaner]
lib/Differentiator/DerivativeBuilder.cpp:52:
- #include <cstddef>
+ #include <clang/Basic/AttrKinds.h>
+ #include <cstddef>| ClonedFunction DerivativeBuilder::cloneFunction(const clang::FunctionDecl* FD, | ||
| clad::VisitorBase& VB, | ||
| clang::DeclContext* DC, | ||
| clang::SourceLocation& noLoc, |
There was a problem hiding this comment.
warning: no header providing "clang::SourceLocation" is directly included [misc-include-cleaner]
lib/Differentiator/DerivativeBuilder.cpp:52:
- #include <cstddef>
+ #include <clang/Basic/SourceLocation.h>
+ #include <cstddef>| clad::VisitorBase& VB, | ||
| clang::DeclContext* DC, | ||
| clang::SourceLocation& noLoc, | ||
| clang::DeclarationNameInfo name, |
There was a problem hiding this comment.
warning: no header providing "clang::DeclarationNameInfo" is directly included [misc-include-cleaner]
lib/Differentiator/DerivativeBuilder.cpp:52:
- #include <cstddef>
+ #include <clang/AST/DeclarationName.h>
+ #include <cstddef>| unsigned NamespaceCount = 0; | ||
| TypeSourceInfo* TSI = m_Context.getTrivialTypeSourceInfo(functionType); | ||
| if (isa<CXXMethodDecl>(FD)) { | ||
| CXXRecordDecl* CXXRD = cast<CXXRecordDecl>(DC); |
There was a problem hiding this comment.
warning: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto]
Suggested change
| CXXRecordDecl* CXXRD = cast<CXXRecordDecl>(DC); | |
| auto* CXXRD = cast<CXXRecordDecl>(DC); |
| // even if their original function had different access specifier. | ||
| returnedFD->setAccess(AS_public); | ||
| } else { | ||
| assert(isa<FunctionDecl>(FD) && "Unexpected!"); |
There was a problem hiding this comment.
warning: no header providing "assert" is directly included [misc-include-cleaner]
lib/Differentiator/DerivativeBuilder.cpp:52:
- #include <cstddef>
+ #include <cassert>
+ #include <cstddef>| assert(isa<FunctionDecl>(FD) && "Unexpected!"); | ||
| NamespaceCount = VB.RebuildEnclosingNamespaces(DC); | ||
|
|
||
| auto TrailingRequiresClause = |
There was a problem hiding this comment.
warning: 'auto TrailingRequiresClause' can be declared as 'auto *TrailingRequiresClause' [llvm-qualified-auto]
Suggested change
| auto TrailingRequiresClause = | |
| auto *TrailingRequiresClause = |
| OverloadExpr* ovl = find.Expression; | ||
|
|
||
| if (isa<UnresolvedLookupExpr>(ovl)) { | ||
| ExprResult result; |
There was a problem hiding this comment.
warning: no header providing "clang::ExprResult" is directly included [misc-include-cleaner]
lib/Differentiator/DerivativeBuilder.cpp:52:
- #include <cstddef>
+ #include <clang/Sema/Ownership.h>
+ #include <cstddef>| SS.Extend(m_Context, NSD, noLoc, noLoc); | ||
| LookupResult DerivativeBuilder::LookupCustomDerivativeOrNumericalDiff( | ||
| const std::string& Name, const clang::DeclContext* originalFnDC, | ||
| CXXScopeSpec& SS, bool forCustomDerv /*=true*/, |
There was a problem hiding this comment.
warning: no header providing "clang::CXXScopeSpec" is directly included [misc-include-cleaner]
lib/Differentiator/DerivativeBuilder.cpp:52:
- #include <cstddef>
+ #include <clang/Sema/DeclSpec.h>
+ #include <cstddef>| bool namespaceShouldExist /*=true*/) { | ||
|
|
||
| IdentifierInfo* II = &m_Context.Idents.get(Name); | ||
| DeclarationName name(II); |
There was a problem hiding this comment.
warning: no header providing "clang::DeclarationName" is directly included [misc-include-cleaner]
DeclarationName name(II);
^| const Expr* Callee = CE->getCallee()->IgnoreParenCasts(); | ||
| if (const auto* DRE = dyn_cast<DeclRefExpr>(Callee)) | ||
| originalFnDC = | ||
| const_cast<DeclContext*>(DRE->getFoundDecl()->getDeclContext()); |
There was a problem hiding this comment.
warning: do not use const_cast to remove const qualifier [cppcoreguidelines-pro-type-const-cast]
const_cast<DeclContext*>(DRE->getFoundDecl()->getDeclContext());
^
Elvand-Lie
force-pushed
the
fix/jacobian-void-return-scalar-fn
branch
from
August 21, 2026 09:11
870f7ba to
75e544e
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/jacobian-void-return-scalar-fn
branch
from
August 21, 2026 09:18
75e544e to
4520092
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Elvand-Lie
force-pushed
the
fix/jacobian-void-return-scalar-fn
branch
from
August 21, 2026 10:01
4520092 to
23412ba
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.
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.
Static-only builds skip the whole test tree because the lit suite and the plugin-compiled unittests load the clad MODULE into a host clang, which an emscripten build cannot provide (LLVM_ENABLE_PLUGINS=OFF). The Basic unittests exercise only the headers, so build them and run them under node, behind a check-clad target through ctest. CLAD_EXTRA_WASM_FLAGS (default -fwasm-exceptions) carries the wasm flags for the test executables; the archives take none.
Cross-compiles clad against the prebuilt llvm-wasm cell (LLVM 22 + emsdk 4.0.9, the artifact CppInterOp's emscripten CI uses) with CLAD_BUILD_STATIC_ONLY=ON and runs the Basic unittests under emsdk's node. Single ubuntu-24.04 row mirroring the most common native config (runtime 22).
A custom pullback had to take its trailing clad::pullback_state<S> by value. That rules out any payload owning storage: a payload holding a clad::tape<T> has no copy constructor, so recording values in the reverse_forw and popping them in the pullback -- the shape a pullback that consumes state instead of replaying the primal needs -- could not be written at all. Let the candidate decide. Whichever form its trailing parameter declares is the one clad expects, so a by-reference pullback matches and a by-value one keeps matching unchanged. The reverse_forw still always takes the carrier by reference; it is an out-parameter it fills during the forward sweep. Both forms are pinned, with the by-value case kept as a regression.
A variable the primal marks private gets an uninitialised copy per thread, which is harmless there because the loop writes it before reading it. Its adjoint is the other way round: the reverse sweep only ever accumulates into it, so every thread began by adding to whatever was on the stack. The uninitialised value reaches the output adjoints -- Memcheck reports it in the gradients OpenMP.C prints, with the origin in the outlined region. Give the adjoint a reduction, as the firstprivate, shared and reduction clauses beside it already do: each thread's copy starts at the identity, which is what accumulating needs. Summing the copies back adds nothing as long as the region writes the variable before reading it, which is what private asks of it. Those three spelled the reduction out one by one, so it moves into a helper on the way past. A reduction needs a type '+' accepts, so an adjoint of any other type stays private and uninitialised as before, and now says so at the pragma rather than leaving a clause that cannot compile in the user's file.
|
clang-tidy review says "All clean, LGTM! 👍" |
A reverse_forw whose mutable arguments all designate storage owned by the calling reverse_forw's own locals records addresses that die with that frame, so no caller ever restores them. clad already recognised this and handed such a call a throwaway tracker -- but a throwaway tracker still performs every store, and the call is taken for no other reason. Suppress the reverse_forw in that case and emit the primal, which does the same work without the records. The forward sweep of a reverse_forw has no reverse sweep to consume anything else the reverse_forw would produce, so nothing else is lost; a call that is taken for its return value or a pullback_state carrier is unaffected. TrackerLocalStorage.C pinned the throwaway tracker itself. It now pins the stronger property: the nested call is emitted as the primal, while storage the caller can still see is recorded as before.
The Valgrind row watches clang and clad compiling, which is where its two hours go, and lit runs it with --leak-check=no. So nothing in CI looks for a leak in the code clad generates -- a tape or an adjoint array a derivative allocates and never frees -- although that is what a change here breaks most directly. The execution tests are already collected in one place: the ExecOnly view re-runs the whole suite with the compile-output checks dropped, so that what is judged is what the programs print. That is the code that knows how a test starts what it built, and so the only place that can put Valgrind in front of it. A new row runs it with leak checking on, against apt's LLVM rather than the from-source recipe the full row needs, since clang is not the process being watched. Wrapping is a substitution, so no test changes and a new test is covered by being written. The view had to be worth running first, having never been run. It swallowed compile output through `xargs -0 true`, which refuses once -ftime-report pushes Misc/TimingsReport.C past ARG_MAX. It reported XPASS for a test whose XFAIL records something about the output the view discards. And the sink it substituted for %filecheck claimed the prefix of %filecheck_exec as well, so what the programs printed was never checked at all -- the one thing this view exists to do. Under all that, ForwardMode/Functors.C leaked. Its functor owns an array by raw pointer, leaves it uninitialised in the default constructor -- which is the one clad calls for the adjoint object -- and had its destructor commented out, so the array was never freed. Initialise it and free it.
The row asked for LLVM 22 while building with clang-20, and the Linux setup installs libomp for the runtime major -- so it looked for libomp-22-dev, which apt-llvm.org does not carry for the newest major, and the OpenMP tests skipped. They are the tests this row is most worth running: what a parallel region does to memory is what nothing else here checks, and the adjoint of a private variable was uninitialised until recently with nothing to catch it. Match the runtime to the compiler, so the libomp the setup installs serves both; which LLVM builds the plugin does not matter to a row that runs Valgrind over the produced programs rather than over clang. Whether the tests run at all was decided by find_package(OpenMP), which probes the compiler CMake was configured with rather than the one lit uses. Where the two disagree, lit promises a feature the tests then fail on with "'omp.h' file not found". Ask the compiler the tests use instead, so a row without libomp skips them, which is what REQUIRES: OpenMP was meant to do. Once they run, the OpenMP runtime's own thread pool shows up: libomp allocates it on first use and holds it until the process exits. Suppress it by the frame directly below malloc, which is libomp's own, so no allocation made by clad or by a test can match the entry.
Every analysis clad grew brought a copy of the same plumbing: a pair of bools on DifferentiationOptions, a pair of branches in ParseArgs, a mutual-exclusion check, a Set<X>AnalysisOptions that differs from its neighbours only in which member it writes, a field on RequestOptions, and a line at each site that hands a derived request its decisions. Adding an analysis means finding all of them, and the copies have already drifted: only TBR is mentioned in -help, and three of the six derived-request sites forget the useful analysis. Describe the analyses once, in Analyses.def, and generate the plumbing from it. The table carries what actually differs between them -- how a user names one, its legacy switch pair, its default, a line of help text -- while what an analysis *is* stays in its own Analyzer. A derived request now inherits its decisions through one helper rather than through whichever fields each site remembered. Forgetting the useful analysis is not observable today: a callee the planner reached already has the flag on its own request, and HandleNestedDiffRequest turns the varied and useful analyses off for dynamically scheduled requests regardless. The dumped derivatives of a forward-mode and a reverse-mode nested call are identical across this change with and without -enable-ua. The only user-visible difference is -help, which now lists every analysis with its default rather than TBR alone.
"-I ${LLVM_INCLUDE_DIRS}" is wrong in both directions, and which way it fails
depends on how LLVM was provided. Against an LLVM build tree the variable names
two directories -- the source include directory and the generated one -- so the
tool is handed "-I a b" and stops with "Too many positional arguments
specified!". In an in-tree build LLVMConfig is never read and the variable is
empty, so the flag swallows the .td that follows it and tablegen reads nothing:
"The class 'Option' is not defined". Only an installed LLVM, where it holds
exactly one directory, happens to work.
Collect the directories from the include path and prepend the flag to each,
which is what llvm's own TableGen.cmake does. This was harmless while the
tables were rendered by a developer target nobody built; now that they are
inputs to compiling clad it breaks the build outright for ROOT, for cling, and
for the compiler-explorer image, which is already red on master.
CMAKE_SOURCE_DIR and CMAKE_BINARY_DIR are clad's only while clad is the top-level project. Added to a bigger build with add_subdirectory, which is what LLVM_EXTERNAL_PROJECTS does, they belong to whoever is on top, and clad's are CLAD_SOURCE_DIR and CLAD_BINARY_DIR. tools/ named the wrong one to find the private tables, so ClangPlugin.cpp stopped at "AnalysisDescs.def: No such file or directory" -- it was looking in the host project's build directory rather than in clad's. It now names CLAD_GEN_LIB, which is where they are rendered. This was invisible while the tables were committed and is a build failure now that they are not; the compiler-explorer image, which builds clad in tree, has been red since. The internal documentation had the same mistake: doxygen would have read the host project's VERSION file, documented its sources, and looked for the tables under its build directory.
Analyses.td said what each analysis is, and CladConfig.h wrote out by hand where it sits in the bitmask a request carries -- alongside the four options that are not analyses at all. A position reaches the mangled name of every request that names it, so it can never be reused or moved, yet the space it is taken from was described in two files. Picking one already spent was a static_assert a file later at best, and the rendered table listed the analyses in an order somebody kept by hand. Put the position in the table, and the four other options with it, so a bit is spoken for in one place. What is rendered is then one sequence in position order, carrying both kinds, and the enum is generated from it rather than kept in step with it. No option changes the bit it has always had; the loop analysis gains a pair by being in the table at all, and nothing yet reads it.
A request starts from what the command line settled on for the translation unit, and three of the four analyses were copied across by hand. An analysis added to Analyses.td would therefore be described everywhere except here: -fenable-analysis=<name> would set it for the translation unit and never reach a request, which is the one place the table's promise would break quietly. Generate the copy from the table. Two exceptions stay written out because they are the semantics rather than the pattern: TBR is dropped where there is no reverse sweep, since a request asking for it there is an error but the command line asking for it everywhere is not; and the loop analysis is also set before error estimation returns, which happens before the rest are seeded.
…nto. clad's public headers expand a table that is rendered into the build directory rather than committed beside them, so code compiled against a source checkout needs that directory on its include path too. %cling stands for a path and carries no flags, so this test names its include paths itself -- and it named only the source tree. Including Differentiator.h therefore stops at "'clad/Differentiator/Analyses.def' file not found", and cling aborts with no output at all, which is what master shows at fbe795a. Pass %clad_gen_incl as well, the substitution the other direct invocations already use. This is the only test that reaches an interpreter or compiler by itself; every other one goes through %clad or %cladclang, whose flags carry that directory. It runs only in the cling row, and that row runs after a merge -- so neither the change that moved the tables into the build directory nor the one that made a public header expand them could see this. Fixes: vgvassilev#2194
A position reaches the mangled name of every request that carries it, so it is never reused and never changes. Nothing enforced that. Picking one another analysis already owned rendered happily and failed a static_assert a file later; picking one of the options CladConfig.h spells out rendered happily too, because the two kinds were described in different files. A position below zero was accepted as well, and being counted from ORDER_BITS it landed in the derivative order, turning a second-order request into an eighteenth-order one with nothing said. clad-tblgen now claims every position, an analysis' pair and a reserved option's single bit alike, and reports the first clash by name together with the lowest free pair to take instead -- which is the thing that otherwise means reading two files. A negative position is rejected outright. Two static_asserts in CladConfig.h cover what the generator cannot see: a table someone hands the headers directly, and a pair that no longer fits the word the bitmask is carried in.
The loop analysis could be switched for a whole translation unit but not for one request: clad::opts carried enable/disable pairs for the other three and none for it, and nothing said so. The three it did carry were read by writing the same four steps out per analysis, and each mismatched pair was reported in the analysis' own words rather than the options the user had written. Read the pairs from Analyses.td instead, in one pass over the same table the command line is generated from, so an analysis that can be switched for the translation unit can be switched for a request by existing at all. The loop analysis gets its pair that way, and a contradictory pair now names what the user wrote, clad::opts::enable_loop and clad::opts::disable_loop. DiffRequest::operator== still compares the analyses by hand and leaves the loop one out, because a pullback does not inherit it from the request that asked for it. vgvassilev#2145 has the repro and what it would take.
A derivative the compiler works out while it compiles was wrong in two ways, and said nothing about either. It read every argument as the first one. clad builds parameters with ParmVarDecl::Create, which leaves the scope index at its default of 0, and FunctionDecl::setParams does not set it either. Codegen assigns arguments by position and never reads that index, which is why run time was always right; constant evaluation keys a parameter's storage on it -- CallRef::getOrigParam in clang's ExprConstant.cpp maps a parameter through OrigCallee->getParamDecl(index) -- so every parameter shared the first one's slot. utils::SetParams now sets the index wherever parameters are attached. And when there was no derivative to call, execute handed back a default-constructed value, which cannot be told from a derivative that really is zero and was reported nowhere. That path now calls NoDerivativeYet, which is deliberately not constexpr, so run time is unchanged and constant evaluation stops with the compiler naming the call. Fixes: vgvassilev#2181
clad's configure looked LLVMTableGen up with find_library(REQUIRED) before adding utils/clad-tblgen, but only one of the two ways that tool can be linked names the result. Without a libLLVM the components are archives and naming the TableGen component is enough, so the lookup decided nothing and could still stop the configure -- which is what it does for ROOT on Windows, where LLVM is a multi-config build tree: the library is in <build>/<config>/lib while LLVM_LIBRARY_DIR names <build>/lib, so it is not found and clad is never built. Move the lookup into the branch that links it. That branch is the one with a libLLVM, and a build tree without one never reaches it, so the configure that was stopping succeeds without the lookup having to learn where a multi-config tree puts its libraries.
Every row is handed an LLVM installation, and one thing only an installation
does is answer LLVM_LIBRARY_DIR with a directory the libraries are in. A
multi-config build tree answers with ".../${CONFIGURATION}/lib" and leaves the
variable unexpanded, so anything looking a library up under that hint finds
nothing. ROOT met that on Windows and clad's whole matrix stayed green, because
no row here is given a build tree at all.
Add the row that is. It is Linux, because the difference is the generator's
rather than MSVC's and Ninja Multi-Config reproduces it, and it configures LLVM
without building it beyond the two libraries clad-tblgen links: LLVMConfig is
written at configure time and is all clad reads, so the failure it exists for
needs no LLVM build. What stays uncovered is MSVC and a build tree together,
which only ROOT builds.
It runs when a file that decides how clad is built changes, and otherwise only
when the label asks -- for which rebuild-needed learns to answer a second
question about the files it already lists.
Whether a derivative is needed while the program compiles follows from where the call asking for it sits, not from anything a user decides, and an option got that wrong in both directions: a call inside a constexpr function without it silently returned zero, and the plugin read the option only in the branch handling forward mode, so clad::gradient never saw it at all. The option also selected a second CladFunction through a requires clause, costing a template parameter and a pair of constructors. DiffCollector now records which function's body holds the call, and the plugin builds the derivative as its declaration group arrives when that function is constexpr. Only such a call site warrants it: building for every constexpr function instead runs clad before the translation unit is complete, which it is not ready for. Every mode gets this, so a reverse-mode gradient evaluates during compilation under C++26; below that the void* adjoint parameters in GradientDerivedFnTraits stop it, which is vgvassilev#2190. A variable the language requires to be initialised by a constant expression is still out of reach, because the compiler diagnoses its initialiser before clad is handed the declaration at all; that is vgvassilev#2188. Such a request also carries a code argument again, so dump() prints the derivative of a constexpr function rather than a placeholder. One constructor now serves both cases, which makes CladFunction a literal type everywhere -- and that is what the requires clause was guarding. clad fills the derivative pointer in by rewriting the call, which happens after the compiler has first tried to work the initialiser of a namespace-scope variable out for itself, so letting that attempt succeed would store the null pointer for good. The constructor refuses to be a constant expression while the pointer is null, which makes the first attempt fail; the value is worked out again after the rewrite and the variable still gets a static initialiser, now holding the real derivative. Misc/GlobalCladFunction.cpp covers it, and nothing else does: the only other test that reaches a namespace-scope CladFunction is Misc/Repl.cpp, which needs clang-repl. Freeing the reserved bit in Analyses.td also moves the lowest free pair of clad::opts positions from 13 to 7, which the table's own checks assert.
…ing. clad::opts::enable_va asks for the activity analysis, enable_ua for the useful one. Nothing but familiarity says so: the spellings are the suffixes the original -enable-va/-disable-va flags used, and -fenable-analysis= and every report the analyses emit have named them activity and useful for a while now. A reader of a request has to know the mapping to know what was asked for. Generate a spelling from the name the analysis goes by, and keep the short one as an alias of it. A position reaches the mangled name of every request that carries it, so the old spellings cannot be dropped and are not: the two are one option under two names, and the test asserts that for every analysis in the table rather than for the two that prompted this. The table now renders the analysis name as a token rather than a string, which is what lets the enumerator be pasted together; the handful of places that wanted it as a string say so with #. The construct names in AnalysisDescs.def are untouched -- those are prose, not identifiers. Rendering a table again when clad-tblgen changes is part of the same change, because without it this one cannot be built: the rules named clad-tblgen-ready and the .td, and the target only orders the tool before the rendering. It does not make the rendering depend on the tool's output, since cmake does not carry the file-level dependencies of a custom target through to what depends on it (cmake #15858 -- llvm's own TableGen.cmake names both the target and the executable for the same reason). So any build that had rendered the old table kept it, and the new macro met the old text: "pasting formed 'enable_\"tbr\"'". Naming the tool's own path fixes it, under both generators clad's rows use.
The compiler-explorer image builds clad with LLVM_EXTERNAL_PROJECTS, where LLVMConfig is never read and the variables it sets are empty, and where CMAKE_SOURCE_DIR and CMAKE_BINARY_DIR belong to LLVM rather than to clad. Two build rules relying on the opposite shipped this week, green across the whole matrix, and the image was red on master until each was found by reading its log. Add the row that builds it that way -- against the checkout rather than against master, which is the part that image cannot do, because its build script clones clad itself. LLVM is compiled rather than installed, since an installed one cannot be built in tree, but not paid for: ci-workflows publishes the objects of that build beside the install, and fetch-ccache reads them. They are read at the commit and the relative path they were produced from, since either one wrong is a cache that misses in silence. What it does not replay are the recipe's own cmake flags, so the step reports the hit rate rather than promising one.
…iser. clad puts a derivative into a call by rewriting that call, and a plugin is handed a declaration only once the compiler has finished with it. A variable the language requires to be initialised by a constant expression is therefore out of reach: the compiler works its initialiser out, and diagnoses it, before clad sees anything. An ordinary variable is fine, because the value is worked out again after the rewrite. What the user saw was clad's internal NoDerivativeYet named in a note, which says nothing about what to write instead. DiagnoseConstantInitRequests reports the call and suggests the form that works: call clad from a constexpr function, keep the result in an ordinary variable there, and evaluate that function where the constant is wanted. It runs from HandleTopLevelDecl whether or not an error has already been reported, since that is the only point clad is given these declarations at all. It covers a constexpr or constinit variable at namespace scope, a constexpr local -- including one inside a constexpr function, where the compiler works it out while the function is still being parsed -- and a variable template, which is reported on the pattern whether or not it is ever instantiated. This is the half of vgvassilev#2188 clad can act on. Removing the restriction would need the derivative to exist before the initialiser is evaluated, and neither ASTConsumer nor ExternalSemaSource offers a hook that early.
A tape push is a placement-new in the common case and a slab allocation once every SLAB_SIZE elements. Both lived in emplace_back, so every push carried the cost of the path it almost never takes, and clang priced clad::push at 260 against a threshold of 250 -- ten points over, which left a call per push in the generated pullbacks. Move the slab growth out of line. The hot path is what callers now inline.
Splitting advance_slab out of emplace_back left the first line of emplace_back's comment sitting above advance_slab, where it describes neither: advance_slab does not add a value, and emplace_back lost the one line that said what it is for. Two lines also kept the indentation they had inside the function they came from.
An indexed read walked the slab list from the head every time, so reading element i past the inline buffer cost (i - 64) / 1024 pointer hops, and reading a tape back in order paid that on every element. The iterator reads by index, so a range-for over a million doubles took 3.7 s. Push and pop never walk, which is why nothing had noticed. A slab is now reached from whichever of the head, the tail and the last access is nearest, and where that landed is kept, so consecutive elements of a sweep are one hop apart at most: the same million doubles take 4 ms, forwards, backwards or through the iterator. The cursor is dropped when pop_back frees the slab it points at, and the multithreaded tape keeps none, since a read there must not write. No derivative changes -- the visitor emits push, pop, back and peek_back, never an index. What this makes possible is a loop's record read by the loop's own counter rather than popped.
Both subscripts reach the non-const at(): the const one casts its object first, and has since before the lookup learned to walk from the nearer end. The const overload rewritten alongside it therefore ran nowhere, and the test named for it was exercising the cast. Remove the overload, and let the test say that the cast is what it covers.
A function with a return before its tail got its reverse sweep as a
`[&]` closure that every return site called. Each local the sweep read
was captured by reference, so its address was taken and it lived in
memory for the whole function, and the sweep stayed a separate function
because the inliner would not copy it into every call site. On the
ATLAS Higgs likelihood in rootbench the hottest pullback returns from a
switch, and the gradient spent its time in exactly that form: 16.9 ms
per evaluation with the activity analysis on, 26.1 ms without.
Now the forward sweep is the closure, handed to clad::forward_sweep,
each early return a return from it, and the reverse follows the call on
every path. The helper's declaration forces the closure inline through
CLAD_FORWARD_SWEEP_ATTRS, which a user can redefine, so every local is
a plain local again. Every declaration after the first early return is
hoisted in front of the closure, since the reverse reaches locals it
never names, through a hoisted pointer or a restore tracker; one whose
initializer reads a local or a parameter is assigned where the original
computed it, never above the guard that protects it. A goto would serve
at the same cost, but a goto is not a constant expression and a return
from a closure is ([expr.const]), so ConstexprReverseMode.cpp now
checks both paths of an early return at compile time.
Two faults the closure uncovered are fixed with it. Sema hands the
consumers every function it instantiates on the spot, so a constexpr
derivative calling the constexpr helper re-entered the plugin's handler
for the node in flight, which built it again without end; the handler
now leaves the graph alone while a node is being built, as
FinalizeTranslationUnit does. And the capture resolver rebuilt a
reference only in the syntactic form of an initializer list while
CodeGen reads the semantic one, so `{x, y}` inside the closure lowered
x as a block-scope static (vgvassilev#1940); it now walks both forms. The ATLAS
gradient drops to 10.7 ms with activity on and 12.5 ms without,
identical to the last digit.
During an assignment, TBR walks an expression's ID sequence to check if the left-hand side is required. Because a pointer field is modeled as a single bit rather than an array of elements, leading to crash
For example:-
struct B {
double* p;
};
void assign(B b) {
b.p[0] = 1.0; // BUG: findReq tries to index past the single bit for 'p'
}
This patch fixes the crash by stopping the walk at the pointer bit and reporting its status directly, preventing out-of-bounds indexing.
A value a loop body stores once per iteration went onto a tape: a push in the forward sweep, a pop in the reverse. Each is a call into an object with slabs behind it, so the optimiser sees a stored value only through memory it cannot follow, and the loop around it neither vectorises nor keeps its locals in registers. On the ATLAS Higgs likelihood in rootbench, where every hot loop runs a literal number of times, the same code with each tape rewritten by hand into an array indexed by the loop compiled a third faster and ran the same, which is where the JIT time of a gradient is lost. The loop analysis now records the count of a loop whose start and bound are both literals, and reports a counted loop that misses it as CLAD1005, the record it keeps on a tape instead. A store in a loop with a literal count takes a slot in an array of that many, `_t[i]`, and a nest of such loops an array with one dimension per loop. The reverse sweep reads the slot by the index, so a loop whose body took a slot steps the index back on every iteration even where nothing else restores it. A slot holds a scalar or a fixed-size array of them, up to a page; anything else, and a loop the analysis could not count, stays on the tape. A slot store used as a value, where a push was a call, is parenthesised, and the delayed store that finalises the value later writes the assignment's right-hand side instead of a call's argument. On ATLAS, with the activity analysis on and each build alone, the gradient's translation unit compiles in 121 s instead of 179 s and the gradient runs in 9.7 ms instead of 10.0, identical to the last digit.
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.
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>
Elvand-Lie
force-pushed
the
fix/jacobian-void-return-scalar-fn
branch
from
October 2, 2026 14:27
624224c to
6fe65d5
Compare
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 addresses upstream Issue vgvassilev#1306 by adding a clear Clad diagnostic error when
clad::jacobianis requested for non-void returning primal functions.Summary of Changes
Clad Diagnostic Error:
JacobianDerivedFnTraitsderives derivative matrix parameters from function arguments and produces a derivative function returningvoid.voidfunction containingreturn _d_x;), producing cryptic compiler errors rather than an informative diagnostic.DerivativeBuilder::Derive()before visitor instantiation, reporting that Jacobian mode does not support non-void return types and instructing users to supply output parameters via pointer, reference, or array arguments.Preserves Custom Jacobians:
Regression Test:
test/Jacobian/NonVoidReturnDiagnostic.Cto verify the diagnostic message output.Ref: Issue vgvassilev#1306