-
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
Initialize derivative pointers that are allocated through malloc or realloc #1124
Initialize derivative pointers that are allocated through malloc or realloc #1124
Conversation
…derivative pointers
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 #1124 +/- ##
==========================================
+ Coverage 94.37% 94.39% +0.02%
==========================================
Files 50 50
Lines 8366 8713 +347
==========================================
+ Hits 7895 8225 +330
- Misses 471 488 +17
... and 30 files with indirect coverage changes
|
clang-tidy review says "All clean, LGTM! 👍" |
@@ -2842,6 +2877,19 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, | |||
ComputeEffectiveDOperands(Ldiff, Rdiff, derivedL, derivedR); | |||
addToCurrentBlock(BuildOp(opCode, derivedL, derivedR), | |||
direction::forward); | |||
if (opCode == BO_Assign && derivedR) |
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.
Why is this required? I thought this PR is only modifying the differentiation of malloc
calls.
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.
I took it one step further and covered realloc as well where there's not a decl stmt, but this may be required for malloc functions also. For example:
double *ptr = nullptr;
ptr = (double *)malloc(10 * sizeof(double));
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.
clang-tidy made some suggestions
clang-tidy review says "All clean, LGTM! 👍" |
@@ -1788,6 +1790,25 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context, | |||
return StmtDiff(); | |||
} | |||
|
|||
// If all arguments are constant literals, then this does not contribute to | |||
// the gradient. | |||
// FIXME: revert this when this is integrated in the activity analysis pass. |
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.
@ovdiiuv, can you comment here?
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.
I thought about that recently. I don't think it has to be reverted, rather modified to track arguments that are elements of constant sets for example. This would resolve #682.
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.
But that should be part of the analysis not here, right? That is, it needs to be implemented on your end.
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.
No, I think it's the right place.
clang-tidy review says "All clean, LGTM! 👍" |
This PR identifies calls to
malloc
orrealloc
and for the derivative pointers it:malloc
withcalloc
calloc
call afterrealloc