Adding support for range-based for loops in the reverse mode #980

ovdiiuv · 2024-07-11T22:27:58Z

Adding basic support for the range-based for loops, does not yet support break and continue statements inside.

Fixes: #723

github-actions

clang-tidy made some suggestions

github-actions · 2024-07-11T22:34:24Z

lib/Differentiator/ReverseModeVisitor.cpp

+                        cast<BinaryOperator>(VisitBegin.getStmt())->getLHS())
+                        ->getDecl()];
+
+    DeclRefExpr* RangeExpr =


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

Suggested change

DeclRefExpr* RangeExpr =

auto* RangeExpr =

github-actions · 2024-07-11T22:34:24Z

lib/Differentiator/ReverseModeVisitor.cpp

+
+    DeclRefExpr* RangeExpr =
+        cast<DeclRefExpr>(cast<BinaryOperator>(VisitRange.getStmt())->getLHS());
+    DeclRefExpr* BeginExpr =


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

Suggested change

DeclRefExpr* BeginExpr =

auto* BeginExpr =

github-actions · 2024-07-11T22:34:24Z

lib/Differentiator/ReverseModeVisitor.cpp

+        BuildOp(BO_Assign, BeginExpr, BuildOp(UO_Deref, RangeExpr));
+    addToCurrentBlock(AssignRange);
+    addToCurrentBlock(AssignBegin);
+    auto* EndDecl = cast<VarDecl>(FRS->getEndStmt()->getSingleDecl());


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

Suggested change

auto* EndDecl = cast<VarDecl>(FRS->getEndStmt()->getSingleDecl());

const auto* EndDecl = cast<VarDecl>(FRS->getEndStmt()->getSingleDecl());

github-actions · 2024-07-11T22:34:24Z

lib/Differentiator/ReverseModeVisitor.cpp

+    Expr* ForwardCond = BuildOp(BO_NE, BeginExpr, EndExpr);
+    // Add item assignment statement to the body.
+    auto* body = utils::PrependAndCreateCompoundStmt(
+        m_Sema.getASTContext(), const_cast<Stmt*>(FRS->getBody()),


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

m_Sema.getASTContext(), const_cast<Stmt*>(FRS->getBody()), ^

github-actions · 2024-07-11T22:34:24Z

lib/Differentiator/ReverseModeVisitor.cpp

+    // Add item assignment statement to the body.
+    auto* body = utils::PrependAndCreateCompoundStmt(
+        m_Sema.getASTContext(), const_cast<Stmt*>(FRS->getBody()),
+        BuildDeclStmt(const_cast<VarDecl*>(Item)));


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

BuildDeclStmt(const_cast<VarDecl*>(Item))); ^

codecov · 2024-07-11T22:35:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.10%. Comparing base (e9c47b7) to head (e3f5a07).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #980      +/-   ##
==========================================
+ Coverage   94.00%   94.10%   +0.09%     
==========================================
  Files          55       55              
  Lines        8139     8225      +86     
==========================================
+ Hits         7651     7740      +89     
+ Misses        488      485       -3

Files	Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h	`97.97% <ø> (ø)`
lib/Differentiator/ReverseModeVisitor.cpp	`97.50% <100.00%> (+0.09%)`	⬆️

... and 2 files with indirect coverage changes

Files	Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h	`97.97% <ø> (ø)`
lib/Differentiator/ReverseModeVisitor.cpp	`97.50% <100.00%> (+0.09%)`	⬆️

... and 2 files with indirect coverage changes

parth-07 · 2024-07-17T20:59:18Z

test/Gradient/Loops.C

+// CHECK-NEXT:     double r = 0;
+// CHECK-NEXT:     double a[3] = {y, x * y, x * x + y};
+// CHECK-NEXT:     _t0 = {{0U|0UL}};
+// CHECK-NEXT:     _d___range1 = &_d_a;


Can we use std::begin and std::end on C-style arrays? If so, then we should probably use these functions instead of explicitly taking the address of the array and using the array size. It will make the functionality reusable and consistent for more complicated types such as std::vector, std::list, and so on...

We could, but the way it is done is more natural, since clad creates all those statements automatically.

How does clad creates statements such as &_d_a automatically? You are synthesizing the statements by making Clang API calls whether you are creating &_d_a or std::begin(_d_a), right?

Yeah, that makes sense. Please correct me if I'm wrong, but that would require the support of the iterators in the reverse mode and clad doesn't support that yet. That's an issue I'm currently working on, but clad doesn't yet support custom pullbacks. If so, I suggest we return on that issue once custom pullbacks are supported.

@parth-07, @ovdiiuv has opened an issue to track that comment so that we can move forward: #1004

vgvassilev · 2024-07-25T20:36:36Z

lib/Differentiator/ReverseModeVisitor.cpp

+    Expr* ForwardCond = BuildOp(BO_NE, BeginExpr, EndExpr);
+    // Add item assignment statement to the body.
+    auto* body = utils::PrependAndCreateCompoundStmt(
+        m_Sema.getASTContext(), const_cast<Stmt*>(FRS->getBody()),


Shouldn’t we clone the body here?

vgvassilev · 2024-07-26T06:08:41Z

lib/Differentiator/ReverseModeVisitor.cpp

+
+    llvm::SaveAndRestore<bool> SaveIsInsideLoop(isInsideLoop);
+    isInsideLoop = true;
+    const auto* Item = FRS->getLoopVariable();


Suggested change

const auto* Item = FRS->getLoopVariable();

VarDecl* Item = FRS->getLoopVariable();

I don't think that would compile, will try later

I bet it will, we should also rename Item to LoopVD or something like that. VD is a common acronym for things like that.

vgvassilev

Thanks for working on this. Seems like it is heading in the right direction.

vgvassilev · 2024-07-26T11:20:12Z

lib/Differentiator/ReverseModeVisitor.cpp

+
+    beginBlock(direction::reverse);
+    // Create all declarations needed.
+    auto* d_BeginDeclRef =


We should spell out the type here. I think it is VarDecl.

vgvassilev · 2024-07-26T11:20:48Z

lib/Differentiator/ReverseModeVisitor.cpp

+    // Create all declarations needed.
+    auto* d_BeginDeclRef =
+        m_Variables[cast<DeclRefExpr>(
+                        cast<BinaryOperator>(VisitBegin.getStmt())->getLHS())


We should move VisitBegin.getStmt())->getLHS() into a temporary variable since it is used more than once.

vgvassilev · 2024-07-26T11:22:09Z

lib/Differentiator/ReverseModeVisitor.cpp

+
+    llvm::SaveAndRestore<bool> SaveIsInsideLoop(isInsideLoop);
+    isInsideLoop = true;
+    const auto* Item = FRS->getLoopVariable();


I bet it will, we should also rename Item to LoopVD or something like that. VD is a common acronym for things like that.

vgvassilev · 2024-07-26T11:22:48Z

lib/Differentiator/ReverseModeVisitor.cpp

+    auto* BeginExpr =
+        cast<DeclRefExpr>(cast<BinaryOperator>(VisitBegin.getStmt())->getLHS());
+
+    auto* RangeInit = Clone(FRS->getRangeInit());


We should describe in a comment, with code example, what are we trying to produce here with the next several blocks of code.

lib/Differentiator/ReverseModeVisitor.cpp

vgvassilev · 2024-07-26T11:26:05Z

lib/Differentiator/ReverseModeVisitor.cpp

+    auto* AssignEnd = BuildDeclStmt(BuildGlobalVarDecl(
+        CloneType(EndDecl->getType()), EndDecl->getNameAsString(),
+        BuildOp(BO_Add, BuildOp(UO_Deref, RangeExpr),
+                cast<BinaryOperator>(EndDecl->getInit())->getRHS()),


These things are better as temporary variables.

vgvassilev · 2024-07-26T11:26:13Z

lib/Differentiator/ReverseModeVisitor.cpp

+    addToCurrentBlock(AssignRange);
+    addToCurrentBlock(AssignBegin);
+    const auto* EndDecl = cast<VarDecl>(FRS->getEndStmt()->getSingleDecl());
+    auto* AssignEnd = BuildDeclStmt(BuildGlobalVarDecl(


Please spell out the type here.

vgvassilev · 2024-07-26T11:28:47Z

lib/Differentiator/ReverseModeVisitor.cpp

+    Expr* ForwardCond = BuildOp(BO_NE, BeginExpr, EndExpr);
+    // Add item assignment statement to the body.
+    auto* body = utils::PrependAndCreateCompoundStmt(
+        m_Sema.getASTContext(), Clone(FRS->getBody()),


The clone operations are better of outside the calls. We need to spell out the type here, too.

vgvassilev · 2024-07-26T11:29:29Z

lib/Differentiator/ReverseModeVisitor.cpp

+    Stmt* Forward =
+        new (m_Context) ForStmt(m_Context, nullptr, ForwardCond, nullptr,
+                                BuildOp(BO_Comma, IncBegin, d_IncBegin),
+                                BodyDiff.getStmt(), noLoc, noLoc, noLoc);


We have a new policy: we try to pass the source locations of the original function instead of noLoc.

github-actions

clang-tidy made some suggestions

lib/Differentiator/ReverseModeVisitor.cpp

github-actions · 2024-07-26T12:26:48Z

lib/Differentiator/ReverseModeVisitor.cpp

+    Stmt* bodyClone = Clone(FRS->getBody());
+    CompoundStmt* body = utils::PrependAndCreateCompoundStmt(
+        m_Sema.getASTContext(), bodyClone,
+        BuildDeclStmt(const_cast<VarDecl*>(LoopVD)));


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

BuildDeclStmt(const_cast<VarDecl*>(LoopVD))); ^

We should clone that variable declaration, too.

vgvassilev · 2024-07-26T13:10:34Z

lib/Differentiator/ReverseModeVisitor.cpp

+        m_Sema.getASTContext(), bodyClone,
+        BuildDeclStmt(const_cast<VarDecl*>(LoopVD)));
+    StmtDiff BodyDiff =
+        DifferentiateLoopBody(body, loopCounter, nullptr, d_DecBegin,


Suggested change

DifferentiateLoopBody(body, loopCounter, nullptr, d_DecBegin,

DifferentiateLoopBody(body, loopCounter, /*condVarDifff=*/nullptr, d_DecBegin,

vgvassilev · 2024-07-26T13:12:50Z

lib/Differentiator/ReverseModeVisitor.cpp

+    Expr* Inc = BuildOp(BO_Comma, IncBegin, d_IncBegin);
+    Stmt* Forward = new (m_Context)
+        ForStmt(m_Context, nullptr, ForwardCond, nullptr, Inc,
+                BodyDiff.getStmt(), FRS->getForLoc(), noLoc, noLoc);


Suggested change

BodyDiff.getStmt(), FRS->getForLoc(), noLoc, noLoc);

BodyDiff.getStmt(), FRS->getForLoc(), FRS->getBeginLoc(), FRS->getEndLoc());

We also need to add comments for the nullptrs on the line above.

vgvassilev · 2024-07-26T13:13:05Z

lib/Differentiator/ReverseModeVisitor.cpp

+    Reverse = endBlock(direction::reverse);
+    Reverse = new (m_Context)
+        ForStmt(m_Context, nullptr, CounterCondition, nullptr, CounterDecrement,
+                Reverse, FRS->getForLoc(), noLoc, noLoc);


test/Gradient/Loops.C

lib/Differentiator/ReverseModeVisitor.cpp

vgvassilev · 2024-07-26T14:00:54Z

lib/Differentiator/ReverseModeVisitor.cpp

+    auto* BeginExpr =
+        cast<DeclRefExpr>(cast<BinaryOperator>(VisitBegin.getStmt())->getLHS());
+
+    auto* RangeInit = Clone(FRS->getRangeInit());


lib/Differentiator/ReverseModeVisitor.cpp

vgvassilev · 2024-07-26T14:03:29Z

lib/Differentiator/ReverseModeVisitor.cpp

+    Stmt* bodyClone = Clone(FRS->getBody());
+    CompoundStmt* body = utils::PrependAndCreateCompoundStmt(
+        m_Sema.getASTContext(), bodyClone,
+        BuildDeclStmt(const_cast<VarDecl*>(LoopVD)));


We should clone that variable declaration, too.

lib/Differentiator/ReverseModeVisitor.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2024-07-26T15:05:28Z

lib/Differentiator/ReverseModeVisitor.cpp

+    Expr* ForwardCond = BuildOp(BO_NE, BeginDeclRef, EndExpr);
+    // Add item assignment statement to the body.
+    Stmt* bodyClone = Clone(FRS->getBody());
+    Stmt* LoopVDStmt = BuildDeclStmt(const_cast<VarDecl*>(LoopVD));


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

Stmt* LoopVDStmt = BuildDeclStmt(const_cast<VarDecl*>(LoopVD)); ^

github-actions · 2024-07-31T16:50:58Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-07-31T16:57:23Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-07-31T17:08:18Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2024-07-31T17:17:10Z

lib/Differentiator/ReverseModeVisitor.cpp

+    if (!LoopVD->getType()->isReferenceType()) {
+      Expr* d_LoopVD = BuildDeclRef(LoopVDDiff.getDecl_dx());
+      AdjLoopVDAddAssign =
+          BuildOp(BO_Assign, d_LoopVD, BuildOp(UO_Deref, d_BeginDeclRef));


Can we add a test case to make codecov happy?

github-actions · 2024-07-31T19:13:41Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-07-31T19:21:04Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-07-31T19:28:19Z

clang-tidy review says "All clean, LGTM! 👍"

Fixes:vgvassilev#723

github-actions · 2024-08-01T14:12:39Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev

LGTM!

github-actions bot reviewed Jul 11, 2024

View reviewed changes

ovdiiuv force-pushed the revrb branch 2 times, most recently from 99dc69b to 1f612ed Compare July 12, 2024 10:22

ovdiiuv requested review from vgvassilev and parth-07 July 12, 2024 10:36

parth-07 reviewed Jul 17, 2024

View reviewed changes

ovdiiuv force-pushed the revrb branch from 1f612ed to 4ab9fad Compare July 24, 2024 20:20

vgvassilev reviewed Jul 25, 2024

View reviewed changes

vgvassilev reviewed Jul 26, 2024

View reviewed changes

ovdiiuv force-pushed the revrb branch from 4ab9fad to f04eccc Compare July 26, 2024 10:51

vgvassilev reviewed Jul 26, 2024

View reviewed changes

ovdiiuv force-pushed the revrb branch 2 times, most recently from 6ac19d4 to a30c75c Compare July 26, 2024 12:19

github-actions bot reviewed Jul 26, 2024

View reviewed changes

ovdiiuv force-pushed the revrb branch from edc2bf0 to 7fcc51d Compare July 26, 2024 12:44

vgvassilev reviewed Jul 26, 2024

View reviewed changes

ovdiiuv force-pushed the revrb branch 2 times, most recently from afd91ee to 04673ac Compare July 26, 2024 13:29

vgvassilev reviewed Jul 26, 2024

View reviewed changes

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved

vgvassilev reviewed Jul 26, 2024

View reviewed changes

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved

ovdiiuv force-pushed the revrb branch from eec4b22 to 1d085ca Compare July 26, 2024 13:57

vgvassilev reviewed Jul 26, 2024

View reviewed changes

ovdiiuv force-pushed the revrb branch 2 times, most recently from f05d9c1 to ef4f9a8 Compare July 26, 2024 14:58

vgvassilev reviewed Jul 26, 2024

View reviewed changes

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved

github-actions bot reviewed Jul 26, 2024

View reviewed changes

ovdiiuv force-pushed the revrb branch 2 times, most recently from 10e92c7 to f3695a7 Compare July 31, 2024 16:34

Fixes: vgvassilev#979

9a1f751

ovdiiuv force-pushed the revrb branch 3 times, most recently from d652fc3 to 9dcec29 Compare July 31, 2024 16:50

ovdiiuv force-pushed the revrb branch from 9dcec29 to 092aa5f Compare July 31, 2024 16:56

ovdiiuv force-pushed the revrb branch from 092aa5f to 99e7bee Compare July 31, 2024 17:01

vgvassilev requested a review from PetroZarytskyi July 31, 2024 17:15

vgvassilev reviewed Jul 31, 2024

View reviewed changes

ovdiiuv force-pushed the revrb branch from 99e7bee to fbf6148 Compare July 31, 2024 19:07

ovdiiuv force-pushed the revrb branch 2 times, most recently from de4093e to 4889eac Compare July 31, 2024 19:20

Adding support for range-based for loops in the reverse mode.

e3f5a07

Fixes:vgvassilev#723

ovdiiuv force-pushed the revrb branch from 4889eac to e3f5a07 Compare August 1, 2024 14:04

vgvassilev approved these changes Aug 1, 2024

View reviewed changes

vgvassilev merged commit 7d1e26c into vgvassilev:master Aug 1, 2024
89 checks passed

	auto* EndDecl = cast<VarDecl>(FRS->getEndStmt()->getSingleDecl());
	const auto* EndDecl = cast<VarDecl>(FRS->getEndStmt()->getSingleDecl());

	const auto* Item = FRS->getLoopVariable();
	VarDecl* Item = FRS->getLoopVariable();

	DifferentiateLoopBody(body, loopCounter, nullptr, d_DecBegin,
	DifferentiateLoopBody(body, loopCounter, /condVarDifff=/nullptr, d_DecBegin,

	BodyDiff.getStmt(), FRS->getForLoc(), noLoc, noLoc);
	BodyDiff.getStmt(), FRS->getForLoc(), FRS->getBeginLoc(), FRS->getEndLoc());

Adding support for range-based for loops in the reverse mode #980

Adding support for range-based for loops in the reverse mode #980

Conversation

ovdiiuv commented Jul 11, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jul 11, 2024

Choose a reason for hiding this comment

github-actions bot Jul 11, 2024

Choose a reason for hiding this comment

github-actions bot Jul 11, 2024

Choose a reason for hiding this comment

github-actions bot Jul 11, 2024

Choose a reason for hiding this comment

github-actions bot Jul 11, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parth-07 Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgvassilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jul 26, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

github-actions bot commented Aug 1, 2024

vgvassilev left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 11, 2024 •

edited

Loading

parth-07 Jul 20, 2024 •

edited

Loading