Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TBR analysis to the reverse mode in Clad. #655

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

PetroZarytskyi
Copy link
Collaborator

This pull request optimizes storing and restoring in the reverse mode of Clad and introduces TBR (To-Be-Recorded) analysis to determine when variables should be stored.

test/Gradient/FunctionCalls.C Outdated Show resolved Hide resolved
test/Gradient/Loops.C Outdated Show resolved Hide resolved
test/Gradient/UserDefinedTypes.C Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 23. Check the log or trigger a new build to see more.

// ConstExprUsage Usage, ASTContext &)
// => bool Expr::EvaluateAsConstantExpr(EvalResult &Result, ASTContext &)

static inline bool Expr_EvaluateAsConstantExpr(const Expr* E,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unused function 'Expr_EvaluateAsConstantExpr' [clang-diagnostic-unused-function]

static inline bool Expr_EvaluateAsConstantExpr(const Expr* E,
                   ^

@@ -41,6 +41,7 @@ namespace clad {
/// the reverse mode we also accumulate Stmts for the reverse pass which
/// will be executed on return.
std::vector<Stmts> m_Reverse;
std::vector<Stmts> m_EssentialReverse;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: member variable 'm_EssentialReverse' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

    std::vector<Stmts> m_EssentialReverse;
                       ^

std::vector<Stmts> m_LoopBlock;
unsigned outputArrayCursor = 0;
unsigned numParams = 0;
bool isVectorValued = false;
bool use_enzyme = false;
bool enableTBR = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: member variable 'enableTBR' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

    bool enableTBR = false;
         ^

include/clad/Differentiator/ReverseModeVisitor.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ReverseModeVisitor.h Outdated Show resolved Hide resolved
// NOLINTBEGIN(cppcoreguidelines-pro-type-union-access)
struct VarData {
enum VarDataType { UNDEFINED, FUND_TYPE, OBJ_TYPE, ARR_TYPE, REF_TYPE };
union VarDataValue {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: class 'VarDataValue' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

    union VarDataValue {
          ^

include/clad/Differentiator/TBRAnalyzer.h Outdated Show resolved Hide resolved
include/clad/Differentiator/TBRAnalyzer.h Outdated Show resolved Hide resolved
}
};
// FIXME: Fix the constness on the callers of this function.
Finder finder(const_cast<clang::Expr*>(E), Exprs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

      Finder finder(const_cast<clang::Expr*>(E), Exprs);
                    ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 13. Check the log or trigger a new build to see more.

lib/Differentiator/CladUtils.cpp Outdated Show resolved Hide resolved
@@ -163,7 +164,7 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context,
// Cast to function pointer.
gradFuncOverloadEPI);

DeclContext* DC = const_cast<DeclContext*>(m_Function->getDeclContext());
auto* DC = const_cast<DeclContext*>(m_Function->getDeclContext());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

    auto* DC = const_cast<DeclContext*>(m_Function->getDeclContext());
               ^

@@ -356,7 +361,7 @@
// Create the gradient function declaration.
llvm::SaveAndRestore<DeclContext*> SaveContext(m_Sema.CurContext);
llvm::SaveAndRestore<Scope*> SaveScope(m_CurScope);
DeclContext* DC = const_cast<DeclContext*>(m_Function->getDeclContext());
auto* DC = const_cast<DeclContext*>(m_Function->getDeclContext());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

    auto* DC = const_cast<DeclContext*>(m_Function->getDeclContext());
               ^

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
if (copyData.type == VarData::FUND_TYPE) {
res.val.m_FundData = copyData.val.m_FundData;
} else if (copyData.type == VarData::OBJ_TYPE || copyData.type == VarData::ARR_TYPE) {
res.val.m_ArrData = std::unique_ptr<ArrMap>(new ArrMap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

lib/Differentiator/TBRAnalyzer.cpp:0:

- #include "clad/Differentiator/TBRAnalyzer.h"
+ #include <memory>
+ 
+ #include "clad/Differentiator/TBRAnalyzer.h"
Suggested change
res.val.m_ArrData = std::unique_ptr<ArrMap>(new ArrMap());
res.val.m_ArrData = std::make_unique<ArrMap>();

val.m_RefData = nullptr;
} else if (utils::isArrayOrPointerType(QT)) {
type = VarData::ARR_TYPE;
val.m_ArrData = std::unique_ptr<ArrMap>(new ArrMap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

Suggested change
val.m_ArrData = std::unique_ptr<ArrMap>(new ArrMap());
val.m_ArrData = std::make_unique<ArrMap>();

lib/Differentiator/TBRAnalyzer.cpp Outdated Show resolved Hide resolved
type = VarData::OBJ_TYPE;
const auto* recordDecl = QT->getAs<RecordType>()->getDecl();
auto& newArrMap = val.m_ArrData;
newArrMap = std::unique_ptr<ArrMap>(new ArrMap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

Suggested change
newArrMap = std::unique_ptr<ArrMap>(new ArrMap());
newArrMap = std::make_unique<ArrMap>();

/// Set current block ID to the ID of entry the block.
auto* entry = &m_CFG->getEntry();
m_CurBlockID = entry->getBlockID();
m_BlockData[m_CurBlockID] = std::unique_ptr<VarsData>(new VarsData());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

Suggested change
m_BlockData[m_CurBlockID] = std::unique_ptr<VarsData>(new VarsData());
m_BlockData[m_CurBlockID] = std::make_unique<VarsData>();

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

for (const clang::CFGElement& Element : block) {
if (Element.getKind() == clang::CFGElement::Statement) {
const clang::Stmt* S = Element.castAs<clang::CFGStmt>().getStmt();
TraverseStmt(const_cast<clang::Stmt*>(S));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

      TraverseStmt(const_cast<clang::Stmt*>(S));
                   ^

/// If the successor doesn't have a VarsData, assign it and attach the
/// current block as previous.
if (!varsData) {
varsData = std::unique_ptr<VarsData>(new VarsData());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

Suggested change
varsData = std::unique_ptr<VarsData>(new VarsData());
varsData = std::make_unique<VarsData>();


auto elseBranch = std::move(m_BlockData[m_CurBlockID]);

m_BlockData[m_CurBlockID] = std::unique_ptr<VarsData>(new VarsData());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

Suggested change
m_BlockData[m_CurBlockID] = std::unique_ptr<VarsData>(new VarsData());
m_BlockData[m_CurBlockID] = std::make_unique<VarsData>();

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 11. Check the log or trigger a new build to see more.

@@ -43,6 +43,8 @@ namespace clad {
bool CallUpdateRequired = false;
/// A flag to enable/disable diag warnings/errors during differentiation.
bool VerboseDiags = false;
/// A flag to enable TBR analysis during reverse-mode differentiation.
bool EnableTBRAnalysis = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for member 'EnableTBRAnalysis' [readability-identifier-naming]

Suggested change
bool EnableTBRAnalysis = false;
bool m_EnableTBRAnalysis = false;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should figure out how to ignore these suggestions for struct with public-only members. @vaithak do we know how to do it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find any way to restrict this for structs only. There is way to have different restrictions for public members, but it will apply to both classes and structs.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess in that case for the public members we should all allow to be prefixed without _m if that's possible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we can add it. Does it make sense to add it in this PR or a separate one?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably as a separate one and we can rebase this one on top.

std::vector<Stmts> m_LoopBlock;
unsigned outputArrayCursor = 0;
unsigned numParams = 0;
bool isVectorValued = false;
bool use_enzyme = false;
bool enableTBR = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for member 'enableTBR' [readability-identifier-naming]

Suggested change
bool enableTBR = false;
bool m_enableTBR = false;

@@ -250,6 +279,12 @@
StmtDiff Result;
bool isConstant;
bool isInsideLoop;
bool needsUpdate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for member 'needsUpdate' [readability-identifier-naming]

Suggested change
bool needsUpdate;
bool m_needsUpdate;

include/clad/Differentiator/ReverseModeVisitor.h:286:

-             isInsideLoop(pIsInsideLoop), needsUpdate(pNeedsUpdate) {}
+             isInsideLoop(pIsInsideLoop), m_needsUpdate(pNeedsUpdate) {}

if (copyData.type == VarData::FUND_TYPE) {
res.val.m_FundData = copyData.val.m_FundData;
} else if (copyData.type == VarData::OBJ_TYPE || copyData.type == VarData::ARR_TYPE) {
res.val.m_ArrData = std::unique_ptr<ArrMap>(new ArrMap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

lib/Differentiator/TBRAnalyzer.cpp:1:

+ 
+ #include <memory>
Suggested change
res.val.m_ArrData = std::unique_ptr<ArrMap>(new ArrMap());
res.val.m_ArrData = std::make_unique<ArrMap>();

Comment on lines +1 to +2
#ifndef CLAD_DIFFERENTIATOR_TBRANALYZER_H
#define CLAD_DIFFERENTIATOR_TBRANALYZER_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: header guard does not follow preferred style [llvm-header-guard]

Suggested change
#ifndef CLAD_DIFFERENTIATOR_TBRANALYZER_H
#define CLAD_DIFFERENTIATOR_TBRANALYZER_H
#ifndef GITHUB_WORKSPACE_LIB_DIFFERENTIATOR_TBRANALYZER_H
#define GITHUB_WORKSPACE_LIB_DIFFERENTIATOR_TBRANALYZER_H

lib/Differentiator/TBRAnalyzer.h:319:

- #endif // CLAD_DIFFERENTIATOR_TBRANALYZER_H
+ #endif // GITHUB_WORKSPACE_LIB_DIFFERENTIATOR_TBRANALYZER_H

/// 'double& x = f(b);' is not supported.
struct VarData {
enum VarDataType { UNDEFINED, FUND_TYPE, OBJ_TYPE, ARR_TYPE, REF_TYPE };
union VarDataValue {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: class 'VarDataValue' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

    union VarDataValue {
          ^

lib/Differentiator/TBRAnalyzer.h Outdated Show resolved Hide resolved
lib/Differentiator/TBRAnalyzer.h Outdated Show resolved Hide resolved
lib/Differentiator/TBRAnalyzer.h Outdated Show resolved Hide resolved
lib/Differentiator/TBRAnalyzer.h Outdated Show resolved Hide resolved
//CHECK-NEXT: else
//CHECK-NEXT: _t1 = t;
//CHECK-NEXT: double &_t2 = (_cond0 ? (t = x) : (t = y));
//CHECK-NEXT: _t3 = t;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have both _t3 and _t4 storing t value?

//CHECK-NEXT: _t2 = y;
//CHECK-NEXT: goto _label0;
//CHECK-NEXT: _label0:
//CHECK-NEXT: _label0:
//CHECK-NEXT: {
//CHECK-NEXT: double _r2 = 1 * _t2;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the new approach, shouldn't this instead be, double _r2 = 1 * y?

Copy link
Collaborator Author

@PetroZarytskyi PetroZarytskyi Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parth-07 I described this problem to you and Vassil earlier. In general, in order to construct dl (which you need to differentiate L), you need differentiated R, but to construct dr (which you need to differentiate R), you need differentiated L. So _t2 instead of y is a temporary easy solution to break this loop.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PetroZarytskyi Oh, yes. I remember this discussion. Can you please tell me if I understand the problem correctly. When generating adjoint code for the below statement:

r = u * SomeExprV

To generate derivative statement _d_u += _d_r * ..., we first need to evaluate the expression SomeExprV. We do not need the derivative of SomeExprV, however, we do need the expression evaluation result to determine if we would benefit from a temporary variable in this case or not.


Ideally, if SomeExprV does not have any side-effects then we can avoid the store-altogether. Can you please open an issue to track this?

test/Gradient/Loops.C Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

if (copyData.m_Type == VarData::FUND_TYPE) {
res.m_Val.m_FundData = copyData.m_Val.m_FundData;
} else if (copyData.m_Type == VarData::OBJ_TYPE || copyData.m_Type == VarData::ARR_TYPE) {
res.m_Val.m_ArrData = std::unique_ptr<ArrMap>(new ArrMap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

lib/Differentiator/TBRAnalyzer.cpp:1:

+ 
+ #include <memory>
Suggested change
res.m_Val.m_ArrData = std::unique_ptr<ArrMap>(new ArrMap());
res.m_Val.m_ArrData = std::make_unique<ArrMap>();

m_Val.m_RefData = nullptr;
} else if (utils::isArrayOrPointerType(QT)) {
m_Type = VarData::ARR_TYPE;
m_Val.m_ArrData = std::unique_ptr<ArrMap>(new ArrMap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

Suggested change
m_Val.m_ArrData = std::unique_ptr<ArrMap>(new ArrMap());
m_Val.m_ArrData = std::make_unique<ArrMap>();


bool DumpSourceFn : 1;
bool DumpSourceFnAST : 1;
bool DumpDerivedFn : 1;
bool DumpDerivedAST : 1;
bool GenerateSourceFile : 1;
bool ValidateClangVersion : 1;
bool EnableTBRAnalysis : 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for member 'EnableTBRAnalysis' [readability-identifier-naming]

tools/ClangPlugin.h:69:

-             ValidateClangVersion(true), EnableTBRAnalysis(false),
+             ValidateClangVersion(true), m_EnableTBRAnalysis(false),
Suggested change
bool EnableTBRAnalysis : 1;
bool m_EnableTBRAnalysis : 1;

tools/ClangPlugin.h:136:

-             m_DO.EnableTBRAnalysis = true;
+             m_DO.m_EnableTBRAnalysis = true;

Copy link

codecov bot commented Nov 24, 2023

Codecov Report

Merging #655 (7a38e51) into master (8647f13) will increase coverage by 0.12%.
The diff coverage is 95.95%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #655      +/-   ##
==========================================
+ Coverage   94.17%   94.30%   +0.12%     
==========================================
  Files          43       45       +2     
  Lines        6334     7003     +669     
==========================================
+ Hits         5965     6604     +639     
- Misses        369      399      +30     
Files Coverage Δ
include/clad/Differentiator/CladUtils.h 100.00% <ø> (ø)
include/clad/Differentiator/Compatibility.h 91.30% <100.00%> (+0.25%) ⬆️
include/clad/Differentiator/DiffPlanner.h 100.00% <ø> (ø)
include/clad/Differentiator/StmtClone.h 77.77% <ø> (ø)
include/clad/Differentiator/VisitorBase.h 100.00% <100.00%> (ø)
lib/Differentiator/ErrorEstimator.cpp 98.50% <100.00%> (-0.06%) ⬇️
lib/Differentiator/StmtClone.cpp 55.96% <100.00%> (+0.73%) ⬆️
lib/Differentiator/TBRAnalyzer.h 100.00% <100.00%> (ø)
lib/Differentiator/VisitorBase.cpp 97.79% <100.00%> (+0.01%) ⬆️
tools/ClangPlugin.cpp 90.25% <100.00%> (+0.15%) ⬆️
... and 5 more
Files Coverage Δ
include/clad/Differentiator/CladUtils.h 100.00% <ø> (ø)
include/clad/Differentiator/Compatibility.h 91.30% <100.00%> (+0.25%) ⬆️
include/clad/Differentiator/DiffPlanner.h 100.00% <ø> (ø)
include/clad/Differentiator/StmtClone.h 77.77% <ø> (ø)
include/clad/Differentiator/VisitorBase.h 100.00% <100.00%> (ø)
lib/Differentiator/ErrorEstimator.cpp 98.50% <100.00%> (-0.06%) ⬇️
lib/Differentiator/StmtClone.cpp 55.96% <100.00%> (+0.73%) ⬆️
lib/Differentiator/TBRAnalyzer.h 100.00% <100.00%> (ø)
lib/Differentiator/VisitorBase.cpp 97.79% <100.00%> (+0.01%) ⬆️
tools/ClangPlugin.cpp 90.25% <100.00%> (+0.15%) ⬆️
... and 5 more

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

@@ -250,6 +262,12 @@
StmtDiff Result;
bool isConstant;
bool isInsideLoop;
bool needsUpdate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for member 'needsUpdate' [readability-identifier-naming]

Suggested change
bool needsUpdate;
bool m_needsUpdate;

include/clad/Differentiator/ReverseModeVisitor.h:269:

-             isInsideLoop(pIsInsideLoop), needsUpdate(pNeedsUpdate) {}
+             isInsideLoop(pIsInsideLoop), m_needsUpdate(pNeedsUpdate) {}

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

std::vector<Stmts> m_LoopBlock;
unsigned outputArrayCursor = 0;
unsigned numParams = 0;
bool isVectorValued = false;
bool use_enzyme = false;
bool enableTBR = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: invalid case style for protected member 'enableTBR' [readability-identifier-naming]

Suggested change
bool enableTBR = false;
bool m_enableTBR = false;

.get();
}
Expr* CallArgs[] = {TapeRef, exprToPush};
Expr* CallArgs[] = {TapeRef, E};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

    Expr* CallArgs[] = {TapeRef, E};
    ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment on lines 2202 to 2177
RDelayed = std::unique_ptr<DelayedStoreResult>(new DelayedStoreResult(DelayedGlobalStoreAndRef(R)));
RResult = RDelayed->Result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

lib/Differentiator/ReverseModeVisitor.cpp:30:

- #include <numeric>
+ #include <memory>
+ #include <numeric>
Suggested change
RDelayed = std::unique_ptr<DelayedStoreResult>(new DelayedStoreResult(DelayedGlobalStoreAndRef(R)));
RResult = RDelayed->Result;
xt) && RisNotConst) {
obalStoreAndRef(R))std::make_unique

@@ -297,9 +298,32 @@ namespace clad {
return clonedStmt;
}
Expr* VisitorBase::Clone(const Expr* E) {
if (auto* DRE = dyn_cast<DeclRefExpr>(E))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'auto *DRE' can be declared as 'const auto *DRE' [readability-qualified-auto]

Suggested change
if (auto* DRE = dyn_cast<DeclRefExpr>(E))
if (const auto* DRE = dyn_cast<DeclRefExpr>(E))

@PetroZarytskyi PetroZarytskyi force-pushed the tbr-to-master branch 2 times, most recently from e5cae86 to 07f0729 Compare November 28, 2023 23:31
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

// If R has no side effects, it can be just cloned
// (no need to store it).
if (R->HasSideEffects(m_Context) && RisNotConst) {
RDelayed = std::unique_ptr<DelayedStoreResult>(new DelayedStoreResult(DelayedGlobalStoreAndRef(R)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

lib/Differentiator/ReverseModeVisitor.cpp:30:

- #include <numeric>
+ #include <memory>
+ #include <numeric>
Suggested change
RDelayed = std::unique_ptr<DelayedStoreResult>(new DelayedStoreResult(DelayedGlobalStoreAndRef(R)));
RDelayed = std::make_unique<DelayedStoreResult>(DelayedGlobalStoreAndRef(R));


bool ReferencesUpdater::VisitDeclRefExpr(DeclRefExpr* DRE) {
// We should only update references of the declarations that were inside
// the original function declaration context.
// Original function = function that we are currently differentiating.
if (!DRE->getDecl()->getDeclContext()->Encloses(m_Function))
return true;

// Replace the declaration if it is present in `m_DeclReplacements`.
if (VarDecl* VD = dyn_cast<VarDecl>(DRE->getDecl())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto]

Suggested change
if (VarDecl* VD = dyn_cast<VarDecl>(DRE->getDecl())) {
if (auto* VD = dyn_cast<VarDecl>(DRE->getDecl())) {

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

void GetInnermostReturnExpr(const clang::Expr* E,
llvm::SmallVectorImpl<clang::Expr*>& Exprs);

bool ContainsFunctionCalls(const clang::Stmt* E);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'clad::utils::ContainsFunctionCalls' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

    bool ContainsFunctionCalls(const clang::Stmt* E);
         ^
Additional context

lib/Differentiator/CladUtils.cpp:621: the definition seen here

    bool ContainsFunctionCalls(const clang::Stmt* S) {
         ^

include/clad/Differentiator/CladUtils.h:325: differing parameters are named here: ('E'), in definition: ('S')

    bool ContainsFunctionCalls(const clang::Stmt* E);
         ^

}
};
CallExprFinder finder;
finder.TraverseStmt(const_cast<Stmt*>(S));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

      finder.TraverseStmt(const_cast<Stmt*>(S));
                          ^

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

void GetInnermostReturnExpr(const clang::Expr* E,
llvm::SmallVectorImpl<clang::Expr*>& Exprs);

bool ContainsFunctionCalls(const clang::Stmt* E);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'clad::utils::ContainsFunctionCalls' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

    bool ContainsFunctionCalls(const clang::Stmt* E);
         ^
Additional context

lib/Differentiator/CladUtils.cpp:619: the definition seen here

    bool ContainsFunctionCalls(const clang::Stmt* S) {
         ^

include/clad/Differentiator/CladUtils.h:325: differing parameters are named here: ('E'), in definition: ('S')

    bool ContainsFunctionCalls(const clang::Stmt* E);
         ^

/*AcceptInvalidDecl=*/false)
.get();
auto* PushDRE = m_Sema
.BuildDeclarationNameExpr(CSS, Push,
/*AcceptInvalidDecl=*/false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: argument name 'AcceptInvalidDecl' in comment does not match parameter name 'NeedsADL' [bugprone-argument-comment]

                                                  /*AcceptInvalidDecl=*/false)
                                                  ^
Additional context

llvm/include/clang/Sema/Sema.h:5138: 'NeedsADL' declared here

                                      bool NeedsADL,
                                           ^

Comment on lines +2192 to +2176
RDelayed = std::unique_ptr<DelayedStoreResult>(
new DelayedStoreResult(DelayedGlobalStoreAndRef(R)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use std::make_unique instead [modernize-make-unique]

lib/Differentiator/ReverseModeVisitor.cpp:30:

- #include <numeric>
+ #include <memory>
+ #include <numeric>
Suggested change
RDelayed = std::unique_ptr<DelayedStoreResult>(
new DelayedStoreResult(DelayedGlobalStoreAndRef(R)));
RDelayed = std::make_unique<DelayedStoreResult>(
DelayedGlobalStoreAndRef(R));

Copy link
Owner

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @PetroZarytskyi! LGTM, please address my last comment and update the commit message text and this should be good to go.

@PetroZarytskyi PetroZarytskyi force-pushed the tbr-to-master branch 3 times, most recently from 6e658c2 to 567d0f9 Compare December 1, 2023 12:33
@vgvassilev vgvassilev force-pushed the tbr-to-master branch 4 times, most recently from 1d180ac to bb5e26d Compare December 1, 2023 15:47
This patch optimizes storing and restoring in the reverse mode of Clad and
introduces TBR analysis to determine when variables should be stored.

Fixes vgvassilev#465, vgvassilev#441, vgvassilev#439, vgvassilev#429. Partially resolves vgvassilev#606.
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment on lines +1 to +2
#ifndef CLAD_DIFFERENTIATOR_TBRANALYZER_H
#define CLAD_DIFFERENTIATOR_TBRANALYZER_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: header guard does not follow preferred style [llvm-header-guard]

Suggested change
#ifndef CLAD_DIFFERENTIATOR_TBRANALYZER_H
#define CLAD_DIFFERENTIATOR_TBRANALYZER_H
#ifndef GITHUB_WORKSPACE_LIB_DIFFERENTIATOR_TBRANALYZER_H
#define GITHUB_WORKSPACE_LIB_DIFFERENTIATOR_TBRANALYZER_H

lib/Differentiator/TBRAnalyzer.h:323:

- #endif // CLAD_DIFFERENTIATOR_TBRANALYZER_H
+ #endif // GITHUB_WORKSPACE_LIB_DIFFERENTIATOR_TBRANALYZER_H

@vgvassilev vgvassilev merged commit 0ab0c6e into vgvassilev:master Dec 1, 2023
74 of 75 checks passed
vgvassilev added a commit that referenced this pull request Dec 3, 2023
When we synthesize the 0 constant we do not take into account if the
corresponding type matches to IntTy. In case it does not match we need to add
the necessary implicit casts.

This patch fixes an issue that became visible after landing PR #655.
vgvassilev added a commit that referenced this pull request Dec 3, 2023
When we synthesize the 0 constant we do not take into account if the
corresponding type matches to IntTy. In case it does not match we need to add
the necessary implicit casts.

This patch fixes an issue that became visible after landing PR #655.
vgvassilev added a commit that referenced this pull request Dec 6, 2023
When we synthesize the 0 constant we do not take into account if the
corresponding type matches to IntTy. In case it does not match we need to add
the necessary implicit casts.

This patch fixes an issue that became visible after landing PR #655.
@PetroZarytskyi PetroZarytskyi deleted the tbr-to-master branch December 11, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants