Store adjoint decls in m_Variables and rebuild references on read. NFC - #1896
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// caching one node that consumers must clone. | ||
| struct AdjointInfo { | ||
| clang::VarDecl* Decl = nullptr; | ||
| enum WrapKind { Plain, Deref, ParenDeref } Wrap = Plain; |
There was a problem hiding this comment.
warning: enum 'WrapKind' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]
enum WrapKind { Plain, Deref, ParenDeref } Wrap = Plain;
^| /// Map used to keep track of variable declarations and match them | ||
| /// with their derivatives. | ||
| std::unordered_map<const clang::ValueDecl*, clang::Expr*> m_Variables; | ||
| std::unordered_map<const clang::ValueDecl*, AdjointInfo> m_Variables; |
There was a problem hiding this comment.
warning: member variable 'm_Variables' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::unordered_map<const clang::ValueDecl*, AdjointInfo> m_Variables;
^| auto derivedPVDName = "_d_vector_" + std::string(PVDII->getName()); | ||
| IdentifierInfo* derivedPVDII = CreateUniqueIdentifier(derivedPVDName); | ||
| Expr* derivedExpr = nullptr; | ||
| VarDecl* adjointDecl = nullptr; |
There was a problem hiding this comment.
warning: no header providing "clang::VarDecl" is directly included [misc-include-cleaner]
VarDecl* adjointDecl = nullptr;
^| // carries for element access elsewhere. | ||
| Expr* paramAssignment = | ||
| BuildOp(BO_Assign, Clone(paramDiff), dVectorParam); | ||
| BuildOp(BO_Assign, buildAdjoint(m_Variables[param])->IgnoreParens(), |
There was a problem hiding this comment.
warning: no header providing "clang::BO_Assign" is directly included [misc-include-cleaner]
BuildOp(BO_Assign, buildAdjoint(m_Variables[param])->IgnoreParens(),
^| return E; | ||
| } | ||
|
|
||
| VisitorBase::AdjointInfo VisitorBase::adjointInfoFrom(Expr* E) { |
There was a problem hiding this comment.
warning: method 'adjointInfoFrom' can be made static [readability-convert-member-functions-to-static]
VisitorBase::AdjointInfo VisitorBase::adjointInfoFrom(Expr* E) {
^| } | ||
| auto* VD = cast<VarDecl>(cast<DeclRefExpr>(Inner)->getDecl()); | ||
| AdjointInfo::WrapKind W = | ||
| Deref ? (Paren ? AdjointInfo::ParenDeref : AdjointInfo::Deref) |
There was a problem hiding this comment.
warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]
Deref ? (Paren ? AdjointInfo::ParenDeref : AdjointInfo::Deref)
^Additional context
lib/Differentiator/VisitorBase.cpp:370: parent conditional operator here
Deref ? (Paren ? AdjointInfo::ParenDeref : AdjointInfo::Deref)
^m_Variables cached one adjoint-reference Expr per variable, and every read cloned it so the shared node was never parented twice. The cached node itself was never parented either -- a dead node per entry -- and the cross-context DeclRef rebuild was duplicated in the forward and reverse readers. Store instead the adjoint's declaration and how the reference wraps it (AdjointInfo: a decl plus plain / *deref / (*deref)) and rebuild a fresh reference on each read through a single buildAdjoint accessor. No node is cached, so none can be shared or left dead, and the two readers share one rebuild path. StmtDiff gains a deferred build for the adjoint slot so a reverse-mode terminal leaf, which never reads the adjoint, allocates nothing. Deletes the standing m_Variables FIXME.
2c10e26 to
0881330
Compare
1 participant
m_Variables cached one adjoint-reference Expr per variable, and every read cloned it so the shared node was never parented twice. The cached node itself was never parented either -- a dead node per entry -- and the cross-context DeclRef rebuild was duplicated in the forward and reverse readers.
Store instead the adjoint's declaration and how the reference wraps it (AdjointInfo: a decl plus plain / *deref / (*deref)) and rebuild a fresh reference on each read through a single buildAdjoint accessor. No node is cached, so none can be shared or left dead, and the two readers share one rebuild path. StmtDiff gains a deferred build for the adjoint slot so a reverse-mode terminal leaf, which never reads the adjoint, allocates nothing. Deletes the standing m_Variables FIXME.