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

Make GlobalStoreAndRef consistent in usage inside and outside of loops #904

Merged
merged 3 commits into from
Jun 1, 2024

Conversation

PetroZarytskyi
Copy link
Collaborator

The purpose of this PR is to make GlobalStoreAndRef work more consistently inside and outside of loops. The issue is well described in #431. The solution proposed in this PR is as follows:
When GlobalStoreAndRef is called inside a loop, it adds a push statement (clad::push(_t0, x)) to the forward block and a pop statement (clad::pop(_t0)) to the reverse block. The return type of GlobalStoreAndRef is changed from StmtDiff to clang::Expr*. The return value is the top element on the tape (clad::back(_t0)). When GlobalStoreAndRef is called outside a loop, the behavior is essentially the same as before except now it returns a single reference to the created variable ref instead of StmtDiff(ref, ref).
For example (when used to store the condition of an if-stmt inside a loop):

 // forward sweep:
clad::push(_cond1, another_choice > 1);
if (clad::back(_cond1)) {
    ...
}
 // reverse sweep:
if (clad::back(_cond1)) {
    ...
}
clad::pop(_cond1);

With this approach, we automatically ensure safe tape pushes/pops when working with return/break/continue statements, which wasn't the case previously. Because of this a lot of the code used to handle those statements got simplified.
Also, the change of the return type makes GlobalStoreAndRef consistent in usage with StoreAndRef.

Note: I made GlobalStoreAndRef return clad::back(_t0) instead of stored values of clad::push(_t0) and clad::pop(_t0) (as proposed by @parth-07 in #431) because in all use cases _r0 = clad::pop(_t0) was added to the reverse block after _r0 was used (because reverse blocks are reversed). Getting the statement order right would make GlobalStoreAndRef a lot harder to use.

Fixes #431.

Copy link
Contributor

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

Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.05%. Comparing base (e3c1ff0) to head (69077c5).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
- Coverage   94.09%   94.05%   -0.04%     
==========================================
  Files          53       53              
  Lines        7805     7722      -83     
==========================================
- Hits         7344     7263      -81     
+ Misses        461      459       -2     
Files Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h 97.84% <ø> (-0.05%) ⬇️
lib/Differentiator/ReverseModeVisitor.cpp 97.16% <100.00%> (-0.02%) ⬇️
Files Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h 97.84% <ø> (-0.05%) ⬇️
lib/Differentiator/ReverseModeVisitor.cpp 97.16% <100.00%> (-0.02%) ⬇️

Copy link
Contributor

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

Copy link
Contributor

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

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

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

Copy link
Collaborator

@parth-07 parth-07 left a comment

Choose a reason for hiding this comment

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

This looks amazing.
I will review it in-depth soon.

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.

This looks good to me. Let's wait for @parth-07's review here.

Copy link
Contributor

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

Copy link
Collaborator

@parth-07 parth-07 left a comment

Choose a reason for hiding this comment

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

This pull-request has simplified a lot of complicated codes. Looks good to me.

@vgvassilev vgvassilev merged commit 8039520 into vgvassilev:master Jun 1, 2024
89 checks passed
@PetroZarytskyi PetroZarytskyi deleted the issue_431 branch June 6, 2024 10:08
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.

Improve consistency of GlobalStoreAndRef
3 participants