Add forward-mode AD support for std::thread. - #1891
Conversation
0bf129e to
9d1e866
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
3b26d88 to
29be48d
Compare
29be48d to
2a51726
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Pull request overview
Adds forward-mode (pushforward) custom-derivative coverage needed for std::thread-based code paths to compile under Clad, and introduces/extends forward-mode tests around concurrency patterns using std::reference_wrapper and std::thread. Also tightens copyability detection and updates forward-mode constructor handling to move out non-copyable ValueAndPushforward results.
Changes:
- Add forward-mode custom derivatives for
std::threadconstruction and basic member ops (join,joinable) inSTLBuiltins.h. - Generalize copyability detection (
isCopyable) to treat deleted/inaccessible copy ctors as non-copyable. - Update forward-mode constructor handling to
std::movenon-copyablevalue/pushforwardmembers out of custom-derivative constructor returns. - Add/extend
ForwardMode/Concurrency.Ctests coveringreference_wrapperandstd::threadusage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/ForwardMode/Concurrency.C | Adds forward-mode tests exercising reference_wrapper and std::thread constructs. |
| lib/Differentiator/CladUtils.cpp | Expands isCopyable to detect inaccessible/deleted copy constructors. |
| lib/Differentiator/BaseForwardModeVisitor.cpp | Moves non-copyable constructor pushforward results to avoid copy attempts. |
| include/clad/Differentiator/STLBuiltins.h | Introduces std::thread pushforwards and additional reference_wrapper pushforwards. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2a51726 to
1973f4e
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
| if (!clonedArgs.empty() && !derivedArgs.empty()) { | ||
| QualType recTy = CE->getType().getNonReferenceType().getCanonicalType(); | ||
| if (const auto* RD = recTy->getAsCXXRecordDecl()) { | ||
| if (RD->getName() == "thread" && RD->isInStdNamespace() && |
There was a problem hiding this comment.
Why do we have to special case here?
There was a problem hiding this comment.
Constructing std::thread doesn’t call the callable, so VisitCallExpr never nest-diffs it. The callable’s tangent is just {}/nullptr, not F_pushforward, which the custom derivative needs to run AD through the worker. We can generalize this later if you prefer.
6207920 to
4b781ab
Compare
4b781ab to
26285b3
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
This adds forward-mode differentiation for std::thread: it finds the
function a thread was started with, generates that function's derivative,
and gives the thread a value/derivative pair. The approach looks right.
I would split it up before landing it.
Scope: this is three changes in one pull request.
The title says std::thread, but about half the diff is
std::reference_wrapper -- the get_pushforward and
operator_T_amp_pushforward overloads in STLBuiltins.h, and the
f_ref, f_cref, f_implicit, f_twice_ref and mul_refs tests. The
isCopyable edit is a third. Each is useful and each can be reviewed on
its own. Three commits would be easier to review and easier to bisect.
Three more findings are on the lines they concern.
What I could not check: I read the change rather than building it, so I do
not know whether the generated code compiles. The test checks 20 runtime
results but only 5 lines of generated code, so a future change to how the
thread pair is built would only be noticed if the numbers moved. I also
could not tell whether the constructor_pushforward overload taking
F&& f, ..., F&& d_f is ever used: F is deduced from both arguments, so
they must have the same type, and I found no test that reaches it.
These comments come from ai-vgvassilev: written by an AI reviewer and posted after @vgvassilev read them.
| // thread). | ||
| for (const clang::CXXConstructorDecl* Ctor : RD->ctors()) | ||
| if (Ctor->isCopyConstructor() && | ||
| (Ctor->isDeleted() || Ctor->getAccess() != clang::AS_public)) |
There was a problem hiding this comment.
This changes a shared function, and nothing in this PR tests the change.
isCopyable now answers "no" for any class whose copy constructor is
private or protected. That is a reasonable answer. The problem is who else
asks: six places in ReverseModeVisitor call isCopyable, and they now
take a different path for every class with a private copy constructor.
This PR only adds forward-mode tests, so nothing here checks reverse mode.
Please move this to its own commit with a reverse-mode test.
| LookupResult R(SemaRef, DN, noLoc, Sema::LookupMemberName); | ||
| R.suppressDiagnostics(); | ||
| SemaRef.LookupQualifiedName(R, RD); | ||
| if (!R.isSingleResult()) |
There was a problem hiding this comment.
clad stays silent when it cannot find the thread's function.
resolveThreadCallable returns nothing for a generic lambda, an
overloaded operator(), a std::function, or a std::bind result. When
that happens the code just skips ahead. No derivative is generated and
nothing is reported. The user gets a wrong number back and no reason to
suspect it.
A wrong answer is worse than an error here. Report it with
DiagnosticsEngine::Report, pointing at the constructor and naming the
callable that could not be resolved.
| pushforwardFnRequest.Functor = MD->getParent(); | ||
| FunctionDecl* pushforwardFD = | ||
| m_Builder.HandleNestedDiffRequest(pushforwardFnRequest); | ||
| if (pushforwardFD && !isa<CXXMethodDecl>(callableFD)) |
There was a problem hiding this comment.
A derivative is generated and then thrown away.
HandleNestedDiffRequest runs for every callable it resolves, but the
result is only used when the callable is not a class method. For a functor
it is dropped. I think that is on purpose: the functor's derivative is
reached later through operator_call_pushforward, so the call is only
here to make clad emit that method.
If so, please say that in a comment. As written it looks like a bug, and
whoever reads it next will either "fix" it or delete it.
Add custom pushforwards in STLBuiltins.h and forward-mode tests.
26285b3 to
e1af565
Compare
5eb9914 to
e2a508b
Compare
|
We still need the reverse of this. |
e2a508b to
4d91545
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
4d91545 to
51dd1ab
Compare
I've started working on it. Should I make a new PR for the reverse mode or extend the current one? |
|
clang-tidy review says "All clean, LGTM! 👍" |
I’d prefer to be a single PR. |
Adds forward-mode custom derivatives for
std::thread(constructors, join, joinable, detach) and extendsForwardMode/Concurrency.C. The worker runs the callable’s pushforward so mutations throughstd::refupdate derivatives (free functions and named functors; lambdas not yet supported). Also generalizesisCopyableand usesstd::movefor non-copyable custom constructor pushforward returns so differentiated code compiles.