-
Notifications
You must be signed in to change notification settings - Fork 122
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
Conversation
709f228
to
efe5676
Compare
clang-tidy review says "All clean, LGTM! 👍" |
efe5676
to
95b12cc
Compare
clang-tidy review says "All clean, LGTM! 👍" |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
... and 1 file with indirect coverage changes
|
There was a problem hiding this 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.
68d4ecf
to
b4c3189
Compare
Hi @vgvassilev! I believe I addressed all your comments. |
clang-tidy review says "All clean, LGTM! 👍" |
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.
b4c3189
to
6679844
Compare
clang-tidy review says "All clean, LGTM! 👍" |
The llvm18 bot seems to have started to fail again. See #994 and llvm/llvm-project#99453 |
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:can be refactored to
Notice: there's also no need to change the adjoint type to
double*
in this case (like was done previously).can be similarly refactored to
_t
variables created byGlobalStoreAndRef
,DelayedGlobalStoreAndRef
, andStoreAndRestore
, e.g.can be refactored to
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.