Add initial support for pointers in reverse mode #686
Conversation
|
@PetroZarytskyi can you provide some comments on what extra needs to be done to make a pointer update statement to be added in the final output when enable-tbr is on? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
vaithak
force-pushed
the
pointer-support
branch
from
c69ebb2
to
5bb4fe8
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
vaithak
force-pushed
the
pointer-support
branch
from
5bb4fe8
to
fdb92ee
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
vaithak
force-pushed
the
pointer-support
branch
from
fdb92ee
to
eaa534a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #686 +/- ##
==========================================
+ Coverage 94.48% 94.51% +0.02%
==========================================
Files 48 48
Lines 7091 7177 +86
==========================================
+ Hits 6700 6783 +83
- Misses 391 394 +3
... and 1 file with indirect coverage changes
|
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Overall it looks good. Can you add more test cases to hit the codecov missing coverage report? Please add more information in the commit log to explain some of the basics. I suspect this commit can close a few bugs that were reported - can you go over them and see if they are fixed and enumerate them in the commit message with the Fixes prefix?
|
clang-tidy review says "All clean, LGTM! 👍" |
vaithak
force-pushed
the
pointer-support
branch
from
d44ce32
to
ada9a18
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
vaithak
force-pushed
the
pointer-support
branch
2 times, most recently
from
2aa5dcb
to
dffbf31
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
vaithak
force-pushed
the
pointer-support
branch
from
dffbf31
to
3028b7c
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
| @@ -22,7 +22,7 @@ template <typename T> class array_ref { | |||
|
|
|||
| public: | |||
| /// Delete default constructor | |||
| array_ref() = delete; | |||
| array_ref() = default; | |||
There was a problem hiding this comment.
Why is this change required?
There was a problem hiding this comment.
If the primal contains an operation of the form arr = arr + 1, where arr is a pointer param, this will result in d_arr = d_arr + 1 statement, where d_arr is of type array_ref.
Now that we are updating this value, this means we need to store its old value and restore it in the reverse pass, thus, requiring a clad::array_ref<T> _t0 initialization, which will require a default constructor.
There was a problem hiding this comment.
Ideally we should not create references out of thin air.
There was a problem hiding this comment.
Ideally, yes. But this is essentially the same restriction as we have for user-defined objects: #627.
vaithak
force-pushed
the
pointer-support
branch
2 times, most recently
from
52d653e
to
c86c6d2
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
vaithak
force-pushed
the
pointer-support
branch
from
1ff15c0
to
fdce3a7
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Can you rebase and squash commits where necessary? |
vaithak
force-pushed
the
pointer-support
branch
from
fdce3a7
to
7d35728
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
updated 👍🏼 |
| // This is not correct, but we need to implement a more advanced analysis | ||
| // to determine which pointer operations are useful to store. | ||
| if (E->getType()->isPointerType()) | ||
| return true; |
There was a problem hiding this comment.
Can we add a test for this branch to make code of happy?
There was a problem hiding this comment.
This is essentially a workaround for some cases of pointers when enableTbr is used, but as mentioned in some comment above, this doesn't cover every case. Adding this to test is essentially adding check with enable-tbr flag for pointer cases, but it will pass for some test and fail for others. Hence, adding it to pointers.c test won't be possible.
There was a problem hiding this comment.
Well in theory we can add a tbr mode test and mark it xfail.
There was a problem hiding this comment.
Will try this out. I have very little idea about xfail.
There was a problem hiding this comment.
Added a new test for TBR analysis with pointers and marked it as xfail for now.
|
A few important points to note:
double arr[2] = {...};
double *u = arr + 1; // arr derivative type is clad::array, therefore, _d_arr + 1 would be element-wise addition!
fn(arr + 0, ...)Two separate causes are responsible for the crash:
I think we should handle these issues separately in subsequent pull requests. This pull request already contains too many significant changes. |
This commit adds support for pointer operation in reverse mode. The technique is to maintain a corresponding derivative pointer variable, which gets updated (and stored/restored) in the exact same way as the primal pointer variable in both forward and reverse passes. Added a workaround (with a FIXME comment) in the UsefulToStoreGlobal method to essentially bypass TBR analysis results for pointer expr. Fixes vgvassilev#195, Fixes vgvassilev#197
vaithak
force-pushed
the
pointer-support
branch
from
7d35728
to
44b6c50
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
This commit adds support for pointer operation in reverse mode. The technique is to maintain a corresponding derivative pointer variable, which gets updated (and stored/restored) in the exact same way as the primal pointer variable in both forward and reverse passes.
Added a workaround (with a FIXME comment) in the UsefulToStoreGlobal method to essentially bypass TBR analysis results for pointer expr.
Fixes #195, Fixes #197