Initial support for memory operations in reverse mode #777

vaithak · 2024-02-19T14:33:17Z

This does not add complete support for new and delete operators, there are many examples that can be constructed which will fail or crash. However, it covers the mostly-used case of allocating memory in the beginning and deallocating in the end.

Edit: Also, added C-style memory alloc and free operations.

fixes #654

codecov · 2024-02-19T14:40:29Z

Codecov Report

Attention: Patch coverage is 98.56115% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 94.78%. Comparing base (4967d9a) to head (e1cf160).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #777      +/-   ##
==========================================
+ Coverage   94.71%   94.78%   +0.06%     
==========================================
  Files          49       49              
  Lines        7342     7477     +135     
==========================================
+ Hits         6954     7087     +133     
- Misses        388      390       +2

Files	Coverage Δ
...clude/clad/Differentiator/BaseForwardModeVisitor.h	`100.00% <ø> (ø)`
include/clad/Differentiator/CladUtils.h	`100.00% <ø> (ø)`
include/clad/Differentiator/Compatibility.h	`91.54% <100.00%> (+0.24%)`	⬆️
include/clad/Differentiator/ReverseModeVisitor.h	`97.87% <ø> (ø)`
lib/Differentiator/BaseForwardModeVisitor.cpp	`99.06% <100.00%> (+0.01%)`	⬆️
lib/Differentiator/ReverseModeVisitor.cpp	`96.55% <100.00%> (+0.16%)`	⬆️
lib/Differentiator/CladUtils.cpp	`96.74% <85.71%> (-0.41%)`	⬇️

... and 1 file with indirect coverage changes

Files	Coverage Δ
...clude/clad/Differentiator/BaseForwardModeVisitor.h	`100.00% <ø> (ø)`
include/clad/Differentiator/CladUtils.h	`100.00% <ø> (ø)`
include/clad/Differentiator/Compatibility.h	`91.54% <100.00%> (+0.24%)`	⬆️
include/clad/Differentiator/ReverseModeVisitor.h	`97.87% <ø> (ø)`
lib/Differentiator/BaseForwardModeVisitor.cpp	`99.06% <100.00%> (+0.01%)`	⬆️
lib/Differentiator/ReverseModeVisitor.cpp	`96.55% <100.00%> (+0.16%)`	⬆️
lib/Differentiator/CladUtils.cpp	`96.74% <85.71%> (-0.41%)`	⬇️

... and 1 file with indirect coverage changes

github-actions

clang-tidy made some suggestions

github-actions · 2024-02-19T14:42:09Z

include/clad/Differentiator/ReverseModeVisitor.h

@@ -41,6 +41,8 @@ namespace clad {
    /// the reverse mode we also accumulate Stmts for the reverse pass which
    /// will be executed on return.
    std::vector<Stmts> m_Reverse;
+    /// Storing expressions to delete/free memory in the reverse pass.
+    Stmts m_DeallocExprs;


warning: member variable 'm_DeallocExprs' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

Stmts m_DeallocExprs; ^

lib/Differentiator/ReverseModeVisitor.cpp

parth-07 · 2024-02-20T20:25:43Z

This seems to be a good start. I think the solution needs some refinements to work for all cases.

The differentiation of the below code fails with the error: free(): double free detected in tcache 2. Aborted (core dumped). The error is correct as we are indeed doing double delete for address in nu and _d_nu.

double fn(double u, double v) {
    double *nu = new double(u);
    double res = 2 * *nu;
    delete nu;
    nu = new double(v);
    res = 2 * *nu;
    delete nu;
    return res;
}

int main() {
    auto fn_grad = clad::gradient(fn);
    double u = 3, v = 5, du = 0, dv = 0;
    fn_grad.execute(u, v, &du, &dv);
}

The current solution is also not equipped to handle delete calls across function boundaries. I am not sure if we want (or need) to support this or not. An example of such a case is:

double identity(double *p) {
    double v = *p;
    delete p;
    return v;
}

double fn(double u, double v) {
    double *nu = new double(u);
    double res = 2 * identity(nu);
    double *nv = new double(v);
    res += 2 * *nv;
    delete nv;
    return res;
}


int main() {
    auto fn_grad = clad::gradient(fn);
    double u = 3, v = 5, du = 0, dv = 0;
    fn_grad.execute(u, v, &du, &dv);
}

The differentiation of this code fails because the derivative pointer would be deleted prematurely in identity_pullback function.

vaithak · 2024-02-21T13:26:02Z

@parth-07 Agreed, this is in no way a perfect solution, but meant as a fix for preventing clad from crashing on new and delete statements. For supporting all the use cases, like the one you mentioned would require a more complex analysis which can be done incrementally when use cases/issues arrive from users.
For example, in the first case, just storing the name of the pointer variable won't be sufficient. As, we can create a new pointer variable pointing to the same location and delete using that one, instead of the one we used while calling new expression.

lib/Differentiator/ReverseModeVisitor.cpp

vgvassilev · 2024-02-21T14:12:57Z

@parth-07 Agreed, this is in no way a perfect solution, but meant as a fix for preventing clad from crashing on new and delete statements. For supporting all the use cases, like the one you mentioned would require a more complex analysis which can be done incrementally when use cases/issues arrive from users. For example, in the first case, just storing the name of the pointer variable won't be sufficient. As, we can create a new pointer variable pointing to the same location and delete using that one, instead of the one we used while calling new expression.

Is there a way to diagnose the unsupported cases?

parth-07 · 2024-02-21T14:21:13Z

For supporting all the use cases, like the one you mentioned would require a more complex analysis which can be done incrementally when use cases/issues arrive from users.

Iteratively adding features when they are required is of course the preferred way. However, we should have an idea on how to support the missing features, otherwise, later on, we may need to change the initial groundwork.

For example, in the first case, just storing the name of the pointer variable won't be sufficient. As, we can create a new pointer variable pointing to the same location and delete using that one, instead of the one we used while calling new expression.

Yes, the first case is easier to resolve. But the second case, delete calls across function boundaries, can be troublesome.

but meant as a fix for preventing clad from crashing on new and delete statements.

If the goal for now is simply to prevent clad from crashing. Then perhaps we can simply ignore the delete statements. I don't think doing manual memory allocation in a mathematical function is a common case.

vaithak · 2024-02-22T12:52:20Z

@parth-07 Agreed, this is in no way a perfect solution, but meant as a fix for preventing clad from crashing on new and delete statements. For supporting all the use cases, like the one you mentioned would require a more complex analysis which can be done incrementally when use cases/issues arrive from users. For example, in the first case, just storing the name of the pointer variable won't be sufficient. As, we can create a new pointer variable pointing to the same location and delete using that one, instead of the one we used while calling new expression.

Is there a way to diagnose the unsupported cases?

One possibility is to show a warning on every delete stmt, something like: clad has limited support for delete stmts, make sure to manually verify the generated code. Or if a warning will be too distracting for every delete, we can add it somewhere in the documentation too. Which one do you think will be better?

vaithak · 2024-02-22T12:55:28Z

If the goal for now is simply to prevent clad from crashing. Then perhaps we can simply ignore the delete statements. I don't think doing manual memory allocation in a mathematical function is a common case.

I think it is better to support it minimally than to not support it at all. I don't see a way of how the analysis of such complex cases will look (even for cases like for loops or if conditions), but even if we have to remove the work in this commit entirely for a better solution in future, I don't see that as an issue.

github-actions

clang-tidy made some suggestions

lib/Differentiator/CladUtils.cpp

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 16. Check the log or trigger a new build to see more.

include/clad/Differentiator/BuiltinDerivatives.h

lib/Differentiator/CladUtils.cpp

github-actions

clang-tidy made some suggestions

include/clad/Differentiator/BuiltinDerivatives.h

parth-07 · 2024-02-22T21:34:28Z

I think it is better to support it minimally than to not support it at all.

We will be able to support the differentiation of functions that use new and delete if we ignore the delete statements. By ignoring delete statement, I mean that no statement will be generated in the adjoint code for a delete statement in the primal code. Differentiation will produce correct results even if there is a double delete or if there's delete across function boundaries. We basically have to choose between:

Ignore delete statements and have correct derivatives at the cost of memory leaks in the generated code.
Generate delete statements in the generated derivative code to avoid memory leaks. In this case, only the basic delete statement usage will work correctly. Any advance usage may result in Clad crashing, or an undefined behavior.

I don't see a way of how the analysis of such complex cases will look (even for cases like for loops or if conditions), but even if we have to remove the work in this commit entirely for a better solution in future, I don't see that as an issue.

Yes, it's not a big issue. However, I believe more effort will be required in this way. I was just giving my two cents. Please continue with the approach that you find best.

vaithak · 2024-02-23T10:28:14Z

We will be able to support the differentiation of functions that use new and delete if we ignore the delete statements. By ignoring delete statement, I mean that no statement will be generated in the adjoint code for a delete statement in the primal code. Differentiation will produce correct results even if there is a double delete or if there's delete across function boundaries. We basically have to choose between:

Ignore delete statements and have correct derivatives at the cost of memory leaks in the generated code.

Generate delete statements in the generated derivative code to avoid memory leaks. In this case, only the basic delete statement usage will work correctly. Any advance usage may result in Clad crashing, or an undefined behavior.

Ah, I misunderstood the ignoring delete statements part, thanks for clarifying. I guess we should let the user have some options to choose from here, some users would prefer one over the other depending on their applications.
So, how about we provide a config option to the user? Something like, -XClang -enable-experimental-dealloc ?

github-actions

clang-tidy made some suggestions

lib/Differentiator/CladUtils.cpp

github-actions · 2024-02-23T11:03:57Z

lib/Differentiator/CladUtils.cpp

+    }
+
+    bool IsMemoryDeallocationFunction(const clang::FunctionDecl* FD) {
+      return FD->getBuiltinID() == Builtin::BIfree;


warning: no member named 'BIfree' in namespace 'clang::Builtin' [clang-diagnostic-error]

return FD->getBuiltinID() == Builtin::BIfree; ^

~~Looks like this was introduced in llvm/llvm-project@7119f76 which is available on clang 15 onward...~~

~~EDIT: This is incorrect. That builtin has been there since clang 7 see (llvm/llvm-project@5ecdb944).~~

Another edit: looks like the BIfree is added in llvm/llvm-project@425a83a5. I guess we need to make sure we have the right ifdef-s so that once we move to more recent versions of the minimal version of supported clang everything becomes uniform.

vgvassilev · 2024-02-23T12:06:37Z

We will be able to support the differentiation of functions that use new and delete if we ignore the delete statements. By ignoring delete statement, I mean that no statement will be generated in the adjoint code for a delete statement in the primal code. Differentiation will produce correct results even if there is a double delete or if there's delete across function boundaries. We basically have to choose between:

Ignore delete statements and have correct derivatives at the cost of memory leaks in the generated code.

Generate delete statements in the generated derivative code to avoid memory leaks. In this case, only the basic delete statement usage will work correctly. Any advance usage may result in Clad crashing, or an undefined behavior.

Ah, I misunderstood the ignoring delete statements part, thanks for clarifying. I guess we should let the user have some options to choose from here, some users would prefer one over the other depending on their applications. So, how about we provide a config option to the user? Something like, -XClang -enable-experimental-dealloc ?

I was wondering if we could emit the delete statements when the allocation is in the function we differentiate and produce a warning if the allocation/deallocation is across function boundaries?

vaithak · 2024-02-24T14:51:45Z

I was wondering if we could emit the delete statements when the allocation is in the function we differentiate and produce a warning if the allocation/deallocation is across function boundaries?

But, there are many cases where even if the delete(s) are in the same function, the current implementation will output wrong statements, for ex. - multiple new and delete combinations (as exemplified by Parth), delete(s) inside for/if conditions and maybe many more.

github-actions

clang-tidy made some suggestions

lib/Differentiator/CladUtils.cpp

vaithak · 2024-02-27T13:16:09Z

Turns out there was some issue when zero initializing a newly created array using new double[2](), it took a good debugging session to figure out the root cause, but this is fixed now.

github-actions

clang-tidy made some suggestions

lib/Differentiator/CladUtils.cpp

lib/Differentiator/ReverseModeVisitor.cpp

vaithak · 2024-02-29T13:06:40Z

All tests are passing finally 🎉

vgvassilev · 2024-02-29T14:29:40Z

test/ForwardMode/Pointer.C

+// CHECK-NEXT:     t->i = i;
+// CHECK-NEXT:     double _d_res = *_d_p + _d_t->i;
+// CHECK-NEXT:     double res = *p + t->i;
+// CHECK-NEXT:     unsigned {{(int|long)}} _t2 = sizeof(double);


I was wondering why we are not writing out size_t here?

not sure about this

Ok, let's discuss that issue separately.

vgvassilev · 2024-02-29T16:41:07Z

@parth-07 and I chatted a little and we'd like to move forward. Once we get some more experience then we should decide if we want to relax the delete operations in favor of more memory leaks.

vgvassilev

LGTM!

github-actions bot reviewed Feb 19, 2024

View reviewed changes

vaithak force-pushed the new-delete branch from 01c8073 to 3d8feec Compare February 19, 2024 14:48

vgvassilev requested a review from parth-07 February 19, 2024 16:11

vgvassilev reviewed Feb 21, 2024

View reviewed changes

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

vaithak changed the title ~~Initial support for new and delete operations in reverse mode~~ Initial support for memory operations in reverse mode Feb 22, 2024

vaithak force-pushed the new-delete branch 2 times, most recently from fee1a93 to dbcf6f7 Compare February 22, 2024 15:00

github-actions bot reviewed Feb 22, 2024

View reviewed changes

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

github-actions bot reviewed Feb 22, 2024

View reviewed changes

vaithak force-pushed the new-delete branch 3 times, most recently from a1f9cd2 to b9f8f31 Compare February 22, 2024 15:57

vgvassilev reviewed Feb 22, 2024

View reviewed changes

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

github-actions bot reviewed Feb 22, 2024

View reviewed changes

vaithak force-pushed the new-delete branch 2 times, most recently from 8f0a34e to af639a0 Compare February 22, 2024 17:10

vaithak force-pushed the new-delete branch from af639a0 to 7e85226 Compare February 23, 2024 10:53

github-actions bot reviewed Feb 23, 2024

View reviewed changes

github-actions bot reviewed Feb 26, 2024

View reviewed changes

vaithak force-pushed the new-delete branch from 0662010 to d78b389 Compare February 26, 2024 15:18

vaithak added 5 commits February 26, 2024 16:21

Initial support for new and delete operations in reverse mode

4ecf347

Fix struct init using initializer lists

3f01278

Add support for CStyleCastExpr

228dbc7

Add support for C-style memory alloc and free in reverse mode AD

df50c3a

Add support for C-style memory alloc and free in forward mode AD

e37039d

vaithak force-pushed the new-delete branch 2 times, most recently from 89e3fde to 9c1f4a1 Compare February 27, 2024 13:14

github-actions bot reviewed Feb 27, 2024

View reviewed changes

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

lib/Differentiator/ReverseModeVisitor.cpp Show resolved Hide resolved

vaithak force-pushed the new-delete branch 4 times, most recently from 11c8e73 to 05b2684 Compare February 29, 2024 12:43

Fix builtin macros and zero init for memory operations

e1cf160

vaithak force-pushed the new-delete branch from 05b2684 to e1cf160 Compare February 29, 2024 13:02

vaithak added this to the v1.5 milestone Feb 29, 2024

vaithak self-assigned this Feb 29, 2024

vgvassilev reviewed Feb 29, 2024

View reviewed changes

vgvassilev approved these changes Feb 29, 2024

View reviewed changes

vgvassilev merged commit 2e7d50a into vgvassilev:master Feb 29, 2024
83 checks passed

vaithak deleted the new-delete branch March 13, 2024 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for memory operations in reverse mode #777

Initial support for memory operations in reverse mode #777

vaithak commented Feb 19, 2024 •

edited

Loading

codecov bot commented Feb 19, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot Feb 19, 2024

parth-07 commented Feb 20, 2024

vaithak commented Feb 21, 2024

vgvassilev commented Feb 21, 2024

parth-07 commented Feb 21, 2024

vaithak commented Feb 22, 2024

vaithak commented Feb 22, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

parth-07 commented Feb 22, 2024

vaithak commented Feb 23, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot Feb 23, 2024

vgvassilev Feb 23, 2024 •

edited

Loading

vgvassilev commented Feb 23, 2024

vaithak commented Feb 24, 2024

github-actions bot left a comment

vaithak commented Feb 27, 2024

github-actions bot left a comment

vaithak commented Feb 29, 2024

vgvassilev Feb 29, 2024

vaithak Feb 29, 2024

vgvassilev Feb 29, 2024

vgvassilev commented Feb 29, 2024

vgvassilev left a comment

Initial support for memory operations in reverse mode #777

Initial support for memory operations in reverse mode #777

Conversation

vaithak commented Feb 19, 2024 • edited Loading

codecov bot commented Feb 19, 2024 • edited Loading

Codecov Report

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Feb 19, 2024

Choose a reason for hiding this comment

parth-07 commented Feb 20, 2024

vaithak commented Feb 21, 2024

vgvassilev commented Feb 21, 2024

parth-07 commented Feb 21, 2024

vaithak commented Feb 22, 2024

vaithak commented Feb 22, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

parth-07 commented Feb 22, 2024

vaithak commented Feb 23, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Feb 23, 2024

Choose a reason for hiding this comment

vgvassilev Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

vgvassilev commented Feb 23, 2024

vaithak commented Feb 24, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

vaithak commented Feb 27, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

vaithak commented Feb 29, 2024

vgvassilev Feb 29, 2024

Choose a reason for hiding this comment

vaithak Feb 29, 2024

Choose a reason for hiding this comment

vgvassilev Feb 29, 2024

Choose a reason for hiding this comment

vgvassilev commented Feb 29, 2024

vgvassilev left a comment

Choose a reason for hiding this comment

vaithak commented Feb 19, 2024 •

edited

Loading

codecov bot commented Feb 19, 2024 •

edited

Loading

vaithak commented Feb 22, 2024 •

edited

Loading

vaithak commented Feb 23, 2024 •

edited

Loading

vgvassilev Feb 23, 2024 •

edited

Loading