Diagnose non-void functions in Jacobian mode - #2001
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
81f3d63 to
a6d8847
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Before I finalize the #1306 diagnostic, I would like to confirm the intended Jacobian return-value contract. The current patch rejects every non-void primal. That handles differentiable scalar returns, for which the generated Jacobian ABI has no return-derivative output slot, but it also rejects status-returning C-style functions such as: I propose the following narrower rule:
This avoids inventing an ABI slot for scalar-return derivatives while preserving common status-code APIs. Would you prefer this selective status-return policy, or should Jacobian primal functions be strictly void-returning? |
a6d8847 to
870f7ba
Compare
|
After reviewing the merged vector-forward Jacobian design and its void-returning derivative ABI, I am keeping this PR scoped to the current void-returning primal contract. Selective status-return support would require separate return-control-flow and output-classification semantics, so I will leave that as follow-up feature work unless maintainers prefer otherwise. |
| 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]
| 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]
| 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());
^870f7ba to
75e544e
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
75e544e to
4520092
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Clad's Jacobian API generates a void derivative function and exposes outputs through pointer, reference, or array parameters. Requesting clad::jacobian for a non-void primal previously proceeded into derivative generation and emitted invalid C++, resulting in a cryptic downstream compiler error. Reject unsupported non-void primal functions in DerivativeBuilder::Derive with a direct diagnostic explaining that Jacobian mode currently requires a void-returning primal and how outputs must be provided. Add regression coverage for scalar returns, output pointers with scalar returns (reproducing Issue vgvassilev#1306), and status-returning functions to document the strict void-returning contract. Signed-off-by: Tempris Admin <elvandlie@gmail.com> Fixes vgvassilev#1306
4520092 to
23412ba
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Description
Clad's Jacobian API generates a void derivative function and exposes outputs through pointer, reference, or array parameters.
Requesting
clad::jacobianfor a non-void primal function previously proceeded into derivative generation and emitted invalid C++, resulting in a cryptic downstream compiler error.This PR adds a direct diagnostic in
DerivativeBuilder::Derivethat rejects non-void primal functions before invalid C++ is generated, explaining that Jacobian mode currently requires a void-returning primal function and that outputs must be provided through pointer, reference, or array parameters.test/Jacobian/NonVoidReturnDiagnostic.Ccovering scalar returns, output pointers with scalar returns (reproducing Jacobian GeneratesreturninvoidFunction #1306), and status-returning functions to document current behavior.Related Issues
Fixes #1306