Add Varied analysis to the reverse mode #1084
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1084 +/- ##
==========================================
+ Coverage 94.25% 94.31% +0.06%
==========================================
Files 55 57 +2
Lines 8445 8608 +163
==========================================
+ Hits 7960 8119 +159
- Misses 485 489 +4
... and 4 files with indirect coverage changes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -55,6 +60,7 @@ struct DiffRequest { | |||
| bool VerboseDiags = false; | |||
| /// A flag to enable TBR analysis during reverse-mode differentiation. | |||
| bool EnableTBRAnalysis = false; | |||
| bool EnableActivityAnalysis = false; | |||
There was a problem hiding this comment.
warning: member variable 'EnableActivityAnalysis' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]
bool EnableActivityAnalysis = false;
^| return curBranch.find(VD) != curBranch.end(); | ||
| } | ||
|
|
||
| void VariedAnalyzer::merge(VarsData* targetData, VarsData* mergeData) { |
There was a problem hiding this comment.
warning: method 'merge' can be made static [readability-convert-member-functions-to-static]
lib/Differentiator/ActivityAnalyzer.h:77:
- void merge(VarsData* targetData, VarsData* mergeData);
+ static void merge(VarsData* targetData, VarsData* mergeData);
| // Set current block ID to the ID of entry the block. | ||
| auto* entry = &m_CFG->getEntry(); | ||
| m_CurBlockID = entry->getBlockID(); | ||
| m_BlockData[m_CurBlockID] = new VarsData(); |
There was a problem hiding this comment.
warning: assigning newly created 'gsl::owner<>' to non-owner 'value_type' (aka 'clad::VariedAnalyzer::VarsData *') [cppcoreguidelines-owning-memory]
m_BlockData[m_CurBlockID] = new VarsData();
^| return curBranch.find(VD) != curBranch.end(); | ||
| } | ||
|
|
||
| void VariedAnalyzer::merge(VarsData* targetData, VarsData* mergeData) { |
There was a problem hiding this comment.
warning: method 'merge' can be made static [readability-convert-member-functions-to-static]
lib/Differentiator/ActivityAnalyzer.h:72:
- void merge(VarsData* targetData, VarsData* mergeData);
+ static void merge(VarsData* targetData, VarsData* mergeData);
| VarsData* varsData2); | ||
|
|
||
| void merge(VarsData* targetData, VarsData* mergeData); | ||
| ASTContext& m_Context; |
There was a problem hiding this comment.
warning: member 'm_Context' of type 'ASTContext &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]
ASTContext& m_Context;
^
ovdiiuv
force-pushed
the
activity
branch
2 times, most recently
from
65075a3
to
df82648
Compare
| return true; | ||
| } | ||
|
|
||
| bool VariedAnalyzer::VisitCallExpr(CallExpr* CE) { return true; } |
There was a problem hiding this comment.
warning: method 'VisitCallExpr' can be made static [readability-convert-member-functions-to-static]
lib/Differentiator/ActivityAnalyzer.h:103:
- bool VisitCallExpr(clang::CallExpr* CE);
+ static bool VisitCallExpr(clang::CallExpr* CE);
ovdiiuv
force-pushed
the
activity
branch
2 times, most recently
from
f5236aa
to
c2ca79b
Compare
There was a problem hiding this comment.
Here is a partial review. Please address the clang-tidy complaints as most of them are good. I'd also rely on @PetroZarytskyi here for a review.
| m_BlockPassCounter.resize(m_CFG->size(), 0); | ||
|
|
||
| // Set current block ID to the ID of entry the block. | ||
| auto* entry = &m_CFG->getEntry(); |
There was a problem hiding this comment.
We should expand the type here.
| } | ||
| } | ||
|
|
||
| for (const auto succ : block.succs()) { |
There was a problem hiding this comment.
We should expand the type here.
| m_Varied = true; | ||
|
|
||
| if (const auto* VD = dyn_cast<VarDecl>(DRE->getDecl())) { | ||
| auto& curBranch = getCurBlockVarsData(); |
There was a problem hiding this comment.
We should expand the type here.
There was a problem hiding this comment.
Thank you for your work. I didn't mention pullbacks and call exprs because we already made huge progress there yesterday. Overall, the PR looks awesome. Its simplicity makes me feel that TBR was overcomplicated. I think we made the right decision to start with a simplified implementation.
| // Set current block ID to the ID of entry the block. | ||
| auto* entry = &m_CFG->getEntry(); | ||
| m_CurBlockID = entry->getBlockID(); | ||
| m_BlockData[m_CurBlockID] = new VarsData(); |
There was a problem hiding this comment.
warning: assigning newly created 'gsl::owner<>' to non-owner 'value_type' (aka 'std::set<const clang::VarDecl *> *') [cppcoreguidelines-owning-memory]
m_BlockData[m_CurBlockID] = new VarsData();
^| auto& succData = m_BlockData[succ->getBlockID()]; | ||
|
|
||
| if (!succData) | ||
| succData = new VarsData(*m_BlockData[block.getBlockID()]); |
There was a problem hiding this comment.
warning: assigning newly created 'gsl::owner<>' to non-owner 'value_type' (aka 'std::set<const clang::VarDecl *> *') [cppcoreguidelines-owning-memory]
succData = new VarsData(*m_BlockData[block.getBlockID()]);
^| return curBranch.find(VD) != curBranch.end(); | ||
| } | ||
|
|
||
| void VariedAnalyzer::merge(VarsData* targetData, VarsData* mergeData) { |
There was a problem hiding this comment.
warning: method 'merge' can be made static [readability-convert-member-functions-to-static]
lib/Differentiator/ActivityAnalyzer.h:28:
- void merge(VarsData* targetData, VarsData* mergeData);
+ static void merge(VarsData* targetData, VarsData* mergeData);
|
|
||
| std::set<const clang::VarDecl*>& m_VariedDecls; | ||
| using VarsData = std::set<const clang::VarDecl*>; | ||
| VarsData* createNewVarsData(VarsData toAssign) { |
There was a problem hiding this comment.
warning: method 'createNewVarsData' can be made static [readability-convert-member-functions-to-static]
| VarsData* createNewVarsData(VarsData toAssign) { | |
| static VarsData* createNewVarsData(VarsData toAssign) { |
| std::set<const clang::VarDecl*>& m_VariedDecls; | ||
| using VarsData = std::set<const clang::VarDecl*>; | ||
| VarsData* createNewVarsData(VarsData toAssign) { | ||
| return new VarsData(toAssign); |
There was a problem hiding this comment.
warning: parameter 'toAssign' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
lib/Differentiator/ActivityAnalyzer.h:13:
+ #include <utility>| return new VarsData(toAssign); | |
| return new VarsData(std::move(toAssign)); |
| std::set<const clang::VarDecl*>& m_VariedDecls; | ||
| using VarsData = std::set<const clang::VarDecl*>; | ||
| VarsData* createNewVarsData(VarsData toAssign) { | ||
| return new VarsData(toAssign); |
There was a problem hiding this comment.
warning: returning a newly created resource of type 'VarsData *' (aka 'set<const clang::VarDecl *> *') or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>' [cppcoreguidelines-owning-memory]
return new VarsData(toAssign);
^
| return curBranch.find(VD) != curBranch.end(); | ||
| } | ||
|
|
||
| void VariedAnalyzer::merge(VarsData* targetData, VarsData* mergeData) { |
| @@ -7,6 +7,11 @@ | |||
| #include "clad/Differentiator/DynamicGraph.h" | |||
| #include "clad/Differentiator/ParseDiffArgsTypes.h" | |||
|
|
|||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
warning: version control conflict marker in file [clang-diagnostic-error]
<<<<<<< HEAD
^| m_Varied = false; | ||
| TraverseStmt(R); | ||
| m_Marking = m_Varied; | ||
| TraverseStmt(L); |
There was a problem hiding this comment.
warning: cannot initialize object parameter of type 'clang::RecursiveASTVisitorclad::VariedAnalyzer' with an expression of type 'clad::VariedAnalyzer' [clang-diagnostic-error]
TraverseStmt(L);
^| TraverseStmt(L); | ||
| m_Marking = false; | ||
| } else { | ||
| TraverseStmt(L); |
There was a problem hiding this comment.
warning: cannot initialize object parameter of type 'clang::RecursiveASTVisitorclad::VariedAnalyzer' with an expression of type 'clad::VariedAnalyzer' [clang-diagnostic-error]
TraverseStmt(L);
^| m_Marking = false; | ||
| } else { | ||
| TraverseStmt(L); | ||
| TraverseStmt(R); |
There was a problem hiding this comment.
warning: cannot initialize object parameter of type 'clang::RecursiveASTVisitorclad::VariedAnalyzer' with an expression of type 'clad::VariedAnalyzer' [clang-diagnostic-error]
TraverseStmt(R);
^
ovdiiuv
force-pushed
the
activity
branch
2 times, most recently
from
23e0923
to
792cd6b
Compare
| /// statements in the reverse mode, improving generated codes efficiency. | ||
| namespace clad { | ||
| using VarsData = std::set<const clang::VarDecl*>; | ||
| static inline void mergeVarsData(VarsData* targetData, VarsData* mergeData) { |
There was a problem hiding this comment.
This function should go in the cpp file.
vgvassilev
force-pushed
the
activity
branch
from
792cd6b
to
63b59dc
Compare
|
Can you rebase this PR? |
| if (isVaried(dyn_cast<VarDecl>(DRE->getDecl()))) | ||
| m_Varied = true; | ||
|
|
||
| if (const auto* VD = dyn_cast<VarDecl>(DRE->getDecl())) { | ||
| if (m_Varied && m_Marking) | ||
| copyVarToCurBlock(VD); | ||
| } |
There was a problem hiding this comment.
| if (isVaried(dyn_cast<VarDecl>(DRE->getDecl()))) | |
| m_Varied = true; | |
| if (const auto* VD = dyn_cast<VarDecl>(DRE->getDecl())) { | |
| if (m_Varied && m_Marking) | |
| copyVarToCurBlock(VD); | |
| } | |
| VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl()); | |
| if (!VD) | |
| return true; | |
| if (isVaried(VD)) | |
| m_Varied = true; | |
| if (m_Varied && m_Marking) | |
| copyVarToCurBlock(VD); |
| std::unique_ptr<clang::CFG> m_CFG; | ||
| std::vector<std::unique_ptr<VarsData>> m_BlockData; | ||
| unsigned m_CurBlockID{}; | ||
| std::set<unsigned> m_CFGQueue; | ||
| bool isVaried(const clang::VarDecl* VD) const; | ||
| void copyVarToCurBlock(const clang::VarDecl* VD); | ||
| VarsData& getCurBlockVarsData() { return *m_BlockData[m_CurBlockID]; } |
There was a problem hiding this comment.
We need documentation here, too.
| VariedAnalyzer& operator=(const VariedAnalyzer&&) = delete; | ||
|
|
||
| /// Runs Varied analysis. | ||
| /// \param FD Function to run the analysis on. |
There was a problem hiding this comment.
| /// \param FD Function to run the analysis on. | |
| /// \param[in] FD Function to run the analysis on. |
| std::set<const clang::VarDecl*>& m_VariedDecls; | ||
| // using VarsData = std::set<const clang::VarDecl*>; | ||
| /// A helper method to allocate VarsData | ||
| /// \param toAssign - Parameter to initialize new VarsData with. |
There was a problem hiding this comment.
| /// \param toAssign - Parameter to initialize new VarsData with. | |
| /// \param toAssign[in] - Parameter to initialize new VarsData with. |
| auto& succData = m_BlockData[succ->getBlockID()]; | ||
|
|
||
| if (!succData) { | ||
| succData = new VarsData(*m_BlockData[block.getBlockID()]); |
There was a problem hiding this comment.
warning: no viable overloaded '=' [clang-diagnostic-error]
succData = new VarsData(*m_BlockData[block.getBlockID()]);
^Additional context
/usr/include/c++/12/bits/unique_ptr.h:405: candidate function not viable: no known conversion from 'VarsData *' (aka 'set<const clang::VarDecl *> *') to 'unique_ptr<set<const VarDecl *>>' for 1st argument
unique_ptr& operator=(unique_ptr&&) = default;
^/usr/include/c++/12/bits/unique_ptr.h:421: candidate template ignored: could not match 'unique_ptr<_Up, _Ep>' against 'VarsData *' (aka 'set<const clang::VarDecl *> *')
operator=(unique_ptr<_Up, _Ep>&& __u) noexcept
^/usr/include/c++/12/bits/unique_ptr.h:431: candidate function not viable: no known conversion from 'VarsData *' (aka 'set<const clang::VarDecl *> *') to 'nullptr_t' (aka 'std::nullptr_t') for 1st argument
operator=(nullptr_t) noexcept
^/usr/include/c++/12/bits/unique_ptr.h:514: candidate function not viable: no known conversion from 'VarsData *' (aka 'set<const clang::VarDecl *> *') to 'const unique_ptr<set<const VarDecl *>>' for 1st argument
unique_ptr& operator=(const unique_ptr&) = delete;
^|
|
||
| if (!succData) { | ||
| succData = new VarsData(*m_BlockData[block.getBlockID()]); | ||
| succData->m_Prev = m_BlockData[block.getBlockID()]; |
There was a problem hiding this comment.
warning: no member named 'm_Prev' in 'std::set<const clang::VarDecl *>' [clang-diagnostic-error]
succData->m_Prev = m_BlockData[block.getBlockID()];
^| return true; | ||
| } | ||
|
|
||
| bool VariedAnalyzer::VisitConditionalOperator(clang::ConditionalOperator* CO) { |
There was a problem hiding this comment.
warning: method 'VisitConditionalOperator' can be made static [readability-convert-member-functions-to-static]
lib/Differentiator/ActivityAnalyzer.h:77:
- bool VisitConditionalOperator(clang::ConditionalOperator* CO);
+ static bool VisitConditionalOperator(clang::ConditionalOperator* CO);| bool VisitUnaryOperator(clang::UnaryOperator* UnOp); | ||
| }; | ||
| } // namespace clad | ||
| #endif // CLAD_DIFFERENTIATOR_ACTIVITYANALYZER_H |
There was a problem hiding this comment.
warning: #endif without #if [clang-diagnostic-error]
#endif // CLAD_DIFFERENTIATOR_ACTIVITYANALYZER_H
^| @@ -2939,7 +2939,7 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, | |||
|
|
|||
| VarDecl* VDClone = nullptr; | |||
| Expr* derivedVDE = nullptr; | |||
| if (VDDerived) | |||
| if (VDDerived && m_DiffReq.shouldHaveAdjoint(const_cast<VarDecl*>(VD))) | |||
There was a problem hiding this comment.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
if (VDDerived && m_DiffReq.shouldHaveAdjoint(const_cast<VarDecl*>(VD)))
^| @@ -2995,8 +2995,15 @@ | |||
| VDDerived->setInitStyle(VarDecl::InitializationStyle::CInit); | |||
| } | |||
| } | |||
| if (!m_DiffReq.shouldHaveAdjoint(const_cast<VarDecl*>(VD))) { | |||
There was a problem hiding this comment.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
if (!m_DiffReq.shouldHaveAdjoint(const_cast<VarDecl*>(VD))) {
^
| for (std::size_t i = 0, e = CE->getNumArgs(); i != e; ++i) { | ||
| clang::Expr* par = CE->getArg(i); | ||
| TraverseStmt(par); | ||
| if (m_Varied || 1) { |
There was a problem hiding this comment.
warning: converting integer literal to bool, use bool literal instead [modernize-use-bool-literals]
| if (m_Varied || 1) { | |
| if (m_Varied || true) { |
There was a problem hiding this comment.
This probably should change to what it was before.
| return true; | ||
| } | ||
|
|
||
| bool VariedAnalyzer::VisitInitListExpr(InitListExpr* ILE) { return true; } |
There was a problem hiding this comment.
warning: method 'VisitInitListExpr' can be made static [readability-convert-member-functions-to-static]
lib/Differentiator/ActivityAnalyzer.h:81:
- bool VisitInitListExpr(clang::InitListExpr* ILE);
+ static bool VisitInitListExpr(clang::InitListExpr* ILE);| @@ -0,0 +1,85 @@ | |||
| #ifndef CLAD_DIFFERENTIATOR_ACTIVITYANALYZER_H | |||
| #define CLAD_DIFFERENTIATOR_ACTIVITYANALYZER_H | |||
There was a problem hiding this comment.
warning: header guard does not follow preferred style [llvm-header-guard]
| #define CLAD_DIFFERENTIATOR_ACTIVITYANALYZER_H | |
| #ifndef GITHUB_WORKSPACE_LIB_DIFFERENTIATOR_ACTIVITYANALYZER_H | |
| #define GITHUB_WORKSPACE_LIB_DIFFERENTIATOR_ACTIVITYANALYZER_H |
lib/Differentiator/ActivityAnalyzer.h:84:
- #endif // CLAD_DIFFERENTIATOR_ACTIVITYANALYZER_H
+ #endif // GITHUB_WORKSPACE_LIB_DIFFERENTIATOR_ACTIVITYANALYZER_H
|
|
||
| bool VariedAnalyzer::VisitDeclStmt(DeclStmt* DS) { | ||
| for (Decl* D : DS->decls()) { | ||
| if (!isa<VarDecl>(D)) |
There was a problem hiding this comment.
A test here could be class C{} c; that produces a DeclStmt whose Stmt is not a VarDecl.
| @@ -55,6 +62,7 @@ struct DiffRequest { | |||
| bool VerboseDiags = false; | |||
| /// A flag to enable TBR analysis during reverse-mode differentiation. | |||
| bool EnableTBRAnalysis = false; | |||
| bool EnableVariedAnalysis = false; | |||
There was a problem hiding this comment.
warning: member variable 'EnableVariedAnalysis' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]
bool EnableVariedAnalysis = false;
^|
|
||
| bool VariedAnalyzer::VisitCallExpr(CallExpr* CE) { | ||
| FunctionDecl* FD = CE->getDirectCallee(); | ||
| bool noHiddenParam = 1; |
There was a problem hiding this comment.
warning: converting integer literal to bool, use bool literal instead [modernize-use-bool-literals]
| bool noHiddenParam = 1; | |
| bool noHiddenParam = true; |
| return true; | ||
| } | ||
|
|
||
| bool VariedAnalyzer::VisitInitListExpr(InitListExpr* ILE) { return true; } |
There was a problem hiding this comment.
warning: method 'VisitInitListExpr' can be made static [readability-convert-member-functions-to-static]
lib/Differentiator/ActivityAnalyzer.h:79:
- bool VisitInitListExpr(clang::InitListExpr* ILE);
+ static bool VisitInitListExpr(clang::InitListExpr* ILE);| if (isVaried(dyn_cast<VarDecl>(DRE->getDecl()))) | ||
| m_Varied = true; | ||
|
|
||
| VarDecl* VD = dyn_cast<VarDecl>(DRE->getDecl()); |
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]
| VarDecl* VD = dyn_cast<VarDecl>(DRE->getDecl()); | |
| auto* VD = dyn_cast<VarDecl>(DRE->getDecl()); |
| /// in a differentiable way. That result enables us to remove redundant | ||
| /// statements in the reverse mode, improving generated codes efficiency. | ||
| namespace clad { | ||
| using VarsData = std::set<const clang::VarDecl*>; |
There was a problem hiding this comment.
This can go in the class definition.
| /// By performing static data-flow analysis, so called Varied variables | ||
| /// are determined, meaning variables that depend on input parameters | ||
| /// in a differentiable way. That result enables us to remove redundant | ||
| /// statements in the reverse mode, improving generated codes efficiency. |
There was a problem hiding this comment.
The documentation should go to the class definition.
| return true; | ||
| } | ||
|
|
||
| bool VariedAnalyzer::VisitInitListExpr(InitListExpr* ILE) { return true; } |
| @@ -40,6 +40,8 @@ void fn1(double i, double j, double* output) { | |||
| // CHECK-NEXT: } | |||
| // CHECK-NEXT: } | |||
|
|
|||
|
|
|||
|
|
|||
| @@ -998,7 +998,7 @@ double sq_defined_later(double x) { | |||
| // CHECK-NEXT: *_d_b += _d_y; | |||
| // CHECK-NEXT: } | |||
|
|
|||
| // CHECK: void do_nothing_pullback(double *u, double *v, double *w, double _d_y, double *_d_u, double *_d_v, double *_d_w) { | |||
| @@ -441,14 +441,14 @@ double do_nothing(double* u, double* v, double* w) { | |||
| return u[0]; | |||
| } | |||
|
|
|||
| // CHECK: void do_nothing_pullback(double *u, double *v, double *w, double _d_y, double *_d_u, double *_d_v, double *_d_w); | |||
There was a problem hiding this comment.
We used to remove non varied parameters from the adjoint, but it'd crash cms example, so I removed it for now.
| } else if (opCode == BO_Add || opCode == BO_Sub || opCode == BO_Mul || | ||
| opCode == BO_Div) { | ||
| for (auto* subexpr : BinOp->children()) | ||
| if (!isa<BinaryOperator>(subexpr)) |
There was a problem hiding this comment.
Why don't we visit binary operators recursively? Doesn't this mean we will ignore a part of x + y + z for example? Since it's treated as a nested addition operation in the AST.
| if (opCode == UO_AddrOf || opCode == UO_Deref) { | ||
| m_Varied = true; | ||
| m_Marking = true; | ||
| } |
There was a problem hiding this comment.
Why do we need to set these modes to true here? And even if so, shouldn't we also set m_Marking to false afterward?
|
|
||
| bool VariedAnalyzer::VisitDeclRefExpr(DeclRefExpr* DRE) { | ||
| if (isVaried(dyn_cast<VarDecl>(DRE->getDecl()))) | ||
| m_Varied = true; |
There was a problem hiding this comment.
Doesn't this portion of the code repeat what comes next? Perhaps we should keep one of these.
| targetData->insert(i); | ||
| for (const clang::VarDecl* i : *targetData) | ||
| mergeData->insert(i); | ||
| } |
There was a problem hiding this comment.
It looks like this function will make targetData and mergeData identical. Do we need them both to be modified? Even if we do, we can probably do *mergedata = *targetdata instead of the last two lines.
This PR adds Varied analysis support in the reverse mode, fixes wrong jacobian results and updates tests.