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

Use initialization instead of assignment when possible in the reverse mode #1013

Merged
merged 3 commits into from
Jul 30, 2024

Conversation

PetroZarytskyi
Copy link
Collaborator

The purpose of this PR is to merge decl stmts (e.g. double _t0;) with their initialization points (e.g._t0 = x;) in the reverse mode whenever this is possible. All of the simplifications happen when the original statement is used on the function global scope because otherwise, the declaration has to be "promoted to the function global scope". The simplification is done in three primary cases:

  1. Adjoints of reference-type variables, e.g.
double *_d_ref = 0;
double &ref = x;
_d_ref = &_d_x;

can be refactored to

double &_d_ref = _d_x;
double &ref = x;

Notice: there's also no need to change the adjoint type to double* in this case (like was done previously).

  1. Adjoints of pointer-type variables, e.g.
double *_d_ptr = 0;
double *ptr = &x;
_d_ptr = &_d_x;

can be similarly refactored to

double *_d_ptr = &_d_x;
double *ptr = &x;
  1. _t variables created by GlobalStoreAndRef, DelayedGlobalStoreAndRef, and StoreAndRestore, e.g.
double _t0;
...
_t0 = x;

can be refactored to

...
double _t0 = x;

Also, after this PR, adjoints on a function global scope are generated right before the original (cloned) variables and not at the very beginning of the function. This was done to make points 1) and 2) work correctly. Adjoints are never used before the original variables so this change will not affect anything.
Fixes #525.

Copy link
Contributor

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

Copy link
Contributor

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

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.00%. Comparing base (10b325a) to head (6679844).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1013      +/-   ##
==========================================
+ Coverage   93.97%   94.00%   +0.03%     
==========================================
  Files          55       55              
  Lines        8096     8139      +43     
==========================================
+ Hits         7608     7651      +43     
  Misses        488      488              
Files Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h 97.97% <100.00%> (+0.10%) ⬆️
lib/Differentiator/ReverseModeVisitor.cpp 97.40% <100.00%> (+0.08%) ⬆️

... and 1 file with indirect coverage changes

Files Coverage Δ
include/clad/Differentiator/ReverseModeVisitor.h 97.97% <100.00%> (+0.10%) ⬆️
lib/Differentiator/ReverseModeVisitor.cpp 97.40% <100.00%> (+0.08%) ⬆️

... and 1 file with indirect coverage changes

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.

LGTM! Some minor comments inline. Please update the commit message with the PR description.

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
@PetroZarytskyi PetroZarytskyi force-pushed the issue-479 branch 2 times, most recently from 68d4ecf to b4c3189 Compare July 30, 2024 12:34
@PetroZarytskyi
Copy link
Collaborator Author

Hi @vgvassilev! I believe I addressed all your comments.

Copy link
Contributor

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

@vgvassilev
Copy link
Owner

Hi @vgvassilev! I believe I addressed all your comments.

Maybe the review comment about the commit messagenot yet?

The purpose of this commit is to merge decl stmts with their initialization points in the reverse mode for pointer- and reference-type variables. All of the simplifications happen when the original statement is used on the function global scope because otherwise, the declaration has to be "promoted to the function global scope". In particular
1) Adjoints of reference-type variables, e.g.
```
double *_d_ref = 0;
double &ref = x;
_d_ref = &_d_x;
```
can be refactored to
```
double &_d_ref = _d_x;
double &ref = x;
```
Notice: there's also no need to change the adjoint type to ``double*`` in this case (like was done previously).

2) Adjoints of pointer-type variables, e.g.
```
double *_d_ptr = 0;
double *ptr = &x;
_d_ptr = &_d_x;
```
can be similarly refactored to
```
double *_d_ptr = &_d_x;
double *ptr = &x;
```

Also, after this commit, adjoints on a function global scope are generated right before the original (cloned) variables and not at the very beginning of the function. Adjoints are never used before the original variables so this change will not affect anything.
…function global scope.

The purpose of this commit is to merge decl stmts (e.g. ``double _t0;``) with their initialization points (e.g.``_t0 = x;``) in the reverse mode whenever this is possible. All of the simplifications happen when the original statement is used on the function global scope because otherwise, the declaration has to be "promoted to the function global scope". The simplification is done for `_t` variables created by `GlobalStoreAndRef`, `DelayedGlobalStoreAndRef`, and `StoreAndRestore`, e.g.
```
double _t0;
...
_t0 = x;
```
can be refactored to
```
...
double _t0 = x;
```
Fixes vgvassilev#525.
Copy link
Contributor

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

@vgvassilev
Copy link
Owner

The llvm18 bot seems to have started to fail again. See #994 and llvm/llvm-project#99453

@vgvassilev vgvassilev merged commit e9c47b7 into vgvassilev:master Jul 30, 2024
88 of 89 checks passed
@PetroZarytskyi PetroZarytskyi deleted the issue-479 branch July 31, 2024 11:28
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 generation of variables and assignemnts
2 participants