From e9af7689bc03ac3eabc70e104e5886b61ae3cee3 Mon Sep 17 00:00:00 2001 From: "petro.zarytskyi" Date: Tue, 18 Jun 2024 16:02:33 +0300 Subject: [PATCH] Use a pointer to store the LHS of assignments with LHS with side effects --- .../clad/Differentiator/ReverseModeVisitor.h | 3 +- lib/Differentiator/DiffPlanner.cpp | 3 +- lib/Differentiator/ReverseModeVisitor.cpp | 64 ++++++++----------- lib/Differentiator/TBRAnalyzer.cpp | 32 +++++----- test/Gradient/Assignments.C | 34 +++++----- 5 files changed, 60 insertions(+), 76 deletions(-) diff --git a/include/clad/Differentiator/ReverseModeVisitor.h b/include/clad/Differentiator/ReverseModeVisitor.h index 76e28c9db..889c0532e 100644 --- a/include/clad/Differentiator/ReverseModeVisitor.h +++ b/include/clad/Differentiator/ReverseModeVisitor.h @@ -254,8 +254,7 @@ namespace clad { clang::Expr* GlobalStoreAndRef(clang::Expr* E, llvm::StringRef prefix = "_t", bool force = false); - StmtDiff StoreAndRestore(clang::Expr* E, llvm::StringRef prefix = "_t", - bool force = false); + StmtDiff StoreAndRestore(clang::Expr* E, llvm::StringRef prefix = "_t"); //// A type returned by DelayedGlobalStoreAndRef /// .Result is a reference to the created (yet uninitialized) global diff --git a/lib/Differentiator/DiffPlanner.cpp b/lib/Differentiator/DiffPlanner.cpp index f74e92c4a..c7ea8af14 100644 --- a/lib/Differentiator/DiffPlanner.cpp +++ b/lib/Differentiator/DiffPlanner.cpp @@ -562,7 +562,8 @@ namespace clad { } bool DiffRequest::shouldBeRecorded(Expr* E) const { - assert(EnableTBRAnalysis && "TBR not enabled!"); + if (!EnableTBRAnalysis) + return true; if (!isa(E) && !isa(E) && !isa(E)) diff --git a/lib/Differentiator/ReverseModeVisitor.cpp b/lib/Differentiator/ReverseModeVisitor.cpp index 551acddd2..0473684d8 100644 --- a/lib/Differentiator/ReverseModeVisitor.cpp +++ b/lib/Differentiator/ReverseModeVisitor.cpp @@ -2033,7 +2033,7 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, Expr* diff_dx = diff.getExpr_dx(); if (isPointerOp) addToCurrentBlock(BuildOp(opCode, diff_dx), direction::forward); - if (UsefulToStoreGlobal(diff.getRevSweepAsExpr())) { + if (m_DiffReq.shouldBeRecorded(E)) { auto op = opCode == UO_PostInc ? UO_PostDec : UO_PostInc; addToCurrentBlock(BuildOp(op, Clone(diff.getRevSweepAsExpr())), direction::reverse); @@ -2050,7 +2050,7 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, Expr* diff_dx = diff.getExpr_dx(); if (isPointerOp) addToCurrentBlock(BuildOp(opCode, diff_dx), direction::forward); - if (UsefulToStoreGlobal(diff.getRevSweepAsExpr())) { + if (m_DiffReq.shouldBeRecorded(E)) { auto op = opCode == UO_PreInc ? UO_PreDec : UO_PreInc; addToCurrentBlock(BuildOp(op, Clone(diff.getRevSweepAsExpr())), direction::reverse); @@ -2332,24 +2332,15 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, // in Lblock beginBlock(direction::reverse); Ldiff = Visit(L, dfdx()); - auto* Lblock = endBlock(direction::reverse); - llvm::SmallVector ExprsToStore; - utils::GetInnermostReturnExpr(Ldiff.getExpr(), ExprsToStore); - - // We need to store values of derivative pointer variables in forward pass - // and restore them in reverse pass. - if (isPointerOp) { - Expr* Edx = Ldiff.getExpr_dx(); - ExprsToStore.push_back(Edx); - } if (L->HasSideEffects(m_Context)) { Expr* E = Ldiff.getExpr(); - auto* storeE = - StoreAndRef(E, m_Context.getLValueReferenceType(E->getType())); - Ldiff.updateStmt(storeE); + auto* storeE = GlobalStoreAndRef(BuildOp(UO_AddrOf, E)); + Ldiff.updateStmt(BuildOp(UO_Deref, storeE)); } + Stmts Lblock = EndBlockWithoutCreatingCS(direction::reverse); + Expr* LCloned = Ldiff.getExpr(); // For x, AssignedDiff is _d_x, for x[i] its _d_x[i], for reference exprs // like (x = y) it propagates recursively, so _d_x is also returned. @@ -2358,16 +2349,23 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, return Clone(BinOp); ResultRef = AssignedDiff; // If assigned expr is dependent, first update its derivative; - auto Lblock_begin = Lblock->body_rbegin(); - auto Lblock_end = Lblock->body_rend(); + if (dfdx() && !Lblock.empty()) { + addToCurrentBlock(*Lblock.begin(), direction::reverse); + Lblock.erase(Lblock.begin()); + } - if (dfdx() && Lblock_begin != Lblock_end) { - addToCurrentBlock(*Lblock_begin, direction::reverse); - Lblock_begin = std::next(Lblock_begin); + // Store the value of the LHS of the assignment in the forward pass + // and restore it in the reverse pass + if (m_DiffReq.shouldBeRecorded(L)) { + StmtDiff pushPop = StoreAndRestore(LCloned); + addToCurrentBlock(pushPop.getExpr(), direction::forward); + addToCurrentBlock(pushPop.getExpr_dx(), direction::reverse); } - for (auto& E : ExprsToStore) { - auto pushPop = StoreAndRestore(E); + // We need to store values of derivative pointer variables in forward pass + // and restore them in reverse pass. + if (isPointerOp) { + StmtDiff pushPop = StoreAndRestore(Ldiff.getExpr_dx()); addToCurrentBlock(pushPop.getExpr(), direction::forward); addToCurrentBlock(pushPop.getExpr_dx(), direction::reverse); } @@ -2469,8 +2467,8 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, opCode); // Output statements from Visit(L). - for (auto it = Lblock_begin; it != Lblock_end; ++it) - addToCurrentBlock(*it, direction::reverse); + for (Stmt* S : Lblock) + addToCurrentBlock(S, direction::reverse); } else if (opCode == BO_Comma) { auto* zero = ConstantFolder::synthesizeLiteral(m_Context.IntTy, m_Context, 0); @@ -2736,8 +2734,7 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, initDiff.getForwSweepExpr_dx())); addToCurrentBlock(assignDerivativeE); if (isInsideLoop) { - StmtDiff pushPop = - StoreAndRestore(derivedVDE, /*prefix=*/"_t", /*force=*/true); + StmtDiff pushPop = StoreAndRestore(derivedVDE); addToCurrentBlock(pushPop.getExpr(), direction::forward); m_LoopBlock.back().push_back(pushPop.getExpr_dx()); } @@ -2922,8 +2919,7 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, auto* declRef = BuildDeclRef(decl); auto* assignment = BuildOp(BO_Assign, declRef, decl->getInit()); if (isInsideLoop) { - auto pushPop = - StoreAndRestore(declRef, /*prefix=*/"_t", /*force=*/true); + auto pushPop = StoreAndRestore(declRef); if (pushPop.getExpr() != declRef) addToCurrentBlock(pushPop.getExpr_dx(), direction::reverse); assignment = BuildOp(BO_Comma, pushPop.getExpr(), assignment); @@ -3104,12 +3100,6 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, if (isa(B)) return false; - // FIXME: Here will be the entry point of the advanced activity analysis. - - // Check if the expression was marked as to-be recorded by an analysis. - if (m_DiffReq.EnableTBRAnalysis) - return m_DiffReq.shouldBeRecorded(B); - // Assume E is useful to store. return true; } @@ -3171,13 +3161,9 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, } StmtDiff ReverseModeVisitor::StoreAndRestore(clang::Expr* E, - llvm::StringRef prefix, - bool force) { + llvm::StringRef prefix) { auto Type = getNonConstType(E->getType(), m_Context, m_Sema); - if (!force && !UsefulToStoreGlobal(E)) - return {}; - if (isInsideLoop) { auto CladTape = MakeCladTapeFor(Clone(E), prefix); Expr* Push = CladTape.Push; diff --git a/lib/Differentiator/TBRAnalyzer.cpp b/lib/Differentiator/TBRAnalyzer.cpp index 72fbb463c..115b16bb8 100644 --- a/lib/Differentiator/TBRAnalyzer.cpp +++ b/lib/Differentiator/TBRAnalyzer.cpp @@ -295,17 +295,7 @@ void TBRAnalyzer::addVar(const clang::VarDecl* VD, bool forceNonRefType) { } void TBRAnalyzer::markLocation(const clang::Expr* E) { - VarData* data = getExprVarData(E); - if (!data || findReq(*data)) { - // FIXME: If any of the data's child nodes are required to store then data - // itself is stored. We might add an option to store separate fields. - // FIXME: Sometimes one location might correspond to multiple stores. For - // example, in ``(x*=y)=u`` x's location will first be marked as required to - // be stored (when passing *= operator) but then marked as not required to - // be stored (when passing = operator). Current method of marking locations - // does not allow to differentiate between these two. - m_TBRLocs.insert(E->getBeginLoc()); - } + m_TBRLocs.insert(E->getBeginLoc()); } void TBRAnalyzer::setIsRequired(const clang::Expr* E, bool isReq) { @@ -703,15 +693,19 @@ bool TBRAnalyzer::VisitBinaryOperator(BinaryOperator* BinOp) { } llvm::SmallVector ExprsToStore; utils::GetInnermostReturnExpr(L, ExprsToStore); + bool hasToBeSetReq = false; for (const auto* innerExpr : ExprsToStore) { - // Mark corresponding SourceLocation as required/not required to be - // stored for all expressions that could be used changed. - markLocation(innerExpr); + // If at least one of ExprsToStore has to be stored, + // mark L as useful to store. + if (VarData* data = getExprVarData(innerExpr)) + hasToBeSetReq = hasToBeSetReq || findReq(*data); // Set them to not required to store because the values were changed. // (if some value was not changed, this could only happen if it was // already not required to store). setIsRequired(innerExpr, /*isReq=*/false); } + if (hasToBeSetReq) + markLocation(L); } else if (opCode == BO_Comma) { setMode(0); TraverseStmt(L); @@ -737,9 +731,13 @@ bool TBRAnalyzer::VisitUnaryOperator(clang::UnaryOperator* UnOp) { llvm::SmallVector ExprsToStore; utils::GetInnermostReturnExpr(E, ExprsToStore); for (const auto* innerExpr : ExprsToStore) { - // Mark corresponding SourceLocation as required/not required to be - // stored for all expressions that could be changed. - markLocation(innerExpr); + // If at least one of ExprsToStore has to be stored, + // mark L as useful to store. + if (VarData* data = getExprVarData(innerExpr)) + if (findReq(*data)) { + markLocation(E); + break; + } } } // FIXME: Ideally, `__real` and `__imag` operators should be treated as member diff --git a/test/Gradient/Assignments.C b/test/Gradient/Assignments.C index 949998f6f..7a010d838 100644 --- a/test/Gradient/Assignments.C +++ b/test/Gradient/Assignments.C @@ -414,19 +414,20 @@ double f9(double x, double y) { //CHECK: void f9_grad(double x, double y, double *_d_x, double *_d_y) { //CHECK-NEXT: double _d_t = 0; //CHECK-NEXT: double _t0; +//CHECK-NEXT: double *_t1; //CHECK-NEXT: double _t2; //CHECK-NEXT: double t = x; //CHECK-NEXT: _t0 = t; -//CHECK-NEXT: double &_t1 = (t *= x); -//CHECK-NEXT: _t2 = t; -//CHECK-NEXT: _t1 *= y; +//CHECK-NEXT: _t1 = &(t *= x); +//CHECK-NEXT: _t2 = *_t1; +//CHECK-NEXT: *_t1 *= y; //CHECK-NEXT: _d_t += 1; //CHECK-NEXT: { -//CHECK-NEXT: t = _t2; +//CHECK-NEXT: *_t1 = _t2; //CHECK-NEXT: double _r_d1 = _d_t; //CHECK-NEXT: _d_t = 0; //CHECK-NEXT: _d_t += _r_d1 * y; -//CHECK-NEXT: *_d_y += _t1 * _r_d1; +//CHECK-NEXT: *_d_y += *_t1 * _r_d1; //CHECK-NEXT: t = _t0; //CHECK-NEXT: double _r_d0 = _d_t; //CHECK-NEXT: _d_t = 0; @@ -473,15 +474,16 @@ double f11(double x, double y) { //CHECK: void f11_grad(double x, double y, double *_d_x, double *_d_y) { //CHECK-NEXT: double _d_t = 0; //CHECK-NEXT: double _t0; +//CHECK-NEXT: double *_t1; //CHECK-NEXT: double _t2; //CHECK-NEXT: double t = x; //CHECK-NEXT: _t0 = t; -//CHECK-NEXT: double &_t1 = (t = x); -//CHECK-NEXT: _t2 = t; -//CHECK-NEXT: _t1 = y; +//CHECK-NEXT: _t1 = &(t = x); +//CHECK-NEXT: _t2 = *_t1; +//CHECK-NEXT: *_t1 = y; //CHECK-NEXT: _d_t += 1; //CHECK-NEXT: { -//CHECK-NEXT: t = _t2; +//CHECK-NEXT: *_t1 = _t2; //CHECK-NEXT: double _r_d1 = _d_t; //CHECK-NEXT: _d_t = 0; //CHECK-NEXT: *_d_y += _r_d1; @@ -504,26 +506,24 @@ double f12(double x, double y) { //CHECK-NEXT: bool _cond0; //CHECK-NEXT: double _t0; //CHECK-NEXT: double _t1; +//CHECK-NEXT: double *_t2; //CHECK-NEXT: double _t3; -//CHECK-NEXT: double _t4; //CHECK-NEXT: double t; //CHECK-NEXT: _cond0 = x > y; //CHECK-NEXT: if (_cond0) //CHECK-NEXT: _t0 = t; //CHECK-NEXT: else //CHECK-NEXT: _t1 = t; -//CHECK-NEXT: double &_t2 = (_cond0 ? (t = x) : (t = y)); -//CHECK-NEXT: _t3 = t; -//CHECK-NEXT: _t4 = t; -//CHECK-NEXT: _t2 *= y; +//CHECK-NEXT: _t2 = &(_cond0 ? (t = x) : (t = y)); +//CHECK-NEXT: _t3 = *_t2; +//CHECK-NEXT: *_t2 *= y; //CHECK-NEXT: _d_t += 1; //CHECK-NEXT: { -//CHECK-NEXT: t = _t3; -//CHECK-NEXT: t = _t4; +//CHECK-NEXT: *_t2 = _t3; //CHECK-NEXT: double _r_d2 = (_cond0 ? _d_t : _d_t); //CHECK-NEXT: (_cond0 ? _d_t : _d_t) = 0; //CHECK-NEXT: (_cond0 ? _d_t : _d_t) += _r_d2 * y; -//CHECK-NEXT: *_d_y += _t2 * _r_d2; +//CHECK-NEXT: *_d_y += *_t2 * _r_d2; //CHECK-NEXT: if (_cond0) { //CHECK-NEXT: t = _t0; //CHECK-NEXT: double _r_d0 = _d_t;