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

Operator overload in reverse mode #619

Merged
merged 1 commit into from
Oct 15, 2023

Conversation

PhrygianGates
Copy link
Contributor

Add support for operator overload in reverse mode

@codecov
Copy link

codecov bot commented Aug 20, 2023

Codecov Report

Merging #619 (578b8fc) into master (41699e8) will increase coverage by 0.04%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
+ Coverage   94.05%   94.09%   +0.04%     
==========================================
  Files          43       43              
  Lines        6237     6269      +32     
==========================================
+ Hits         5866     5899      +33     
+ Misses        371      370       -1     
Files Coverage Δ
...e/clad/Differentiator/ReverseModeForwPassVisitor.h 100.00% <ø> (ø)
include/clad/Differentiator/ReverseModeVisitor.h 98.70% <ø> (ø)
lib/Differentiator/ReverseModeForwPassVisitor.cpp 100.00% <100.00%> (ø)
lib/Differentiator/ReverseModeVisitor.cpp 95.86% <100.00%> (+0.08%) ⬆️
Files Coverage Δ
...e/clad/Differentiator/ReverseModeForwPassVisitor.h 100.00% <ø> (ø)
include/clad/Differentiator/ReverseModeVisitor.h 98.70% <ø> (ø)
lib/Differentiator/ReverseModeForwPassVisitor.cpp 100.00% <100.00%> (ø)
lib/Differentiator/ReverseModeVisitor.cpp 95.86% <100.00%> (+0.08%) ⬆️

Copy link
Contributor

@github-actions github-actions bot left a 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

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

include/clad/Differentiator/ReverseModeForwPassVisitor.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ReverseModeForwPassVisitor.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ReverseModeForwPassVisitor.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ReverseModeForwPassVisitor.h Outdated Show resolved Hide resolved
lib/Differentiator/DerivativeBuilder.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a 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

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

lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved
@PhrygianGates PhrygianGates force-pushed the operator_overload_rev branch from b18b297 to 34ef818 Compare August 28, 2023 16:58
Copy link
Contributor

@github-actions github-actions bot left a 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

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

lib/Differentiator/DerivativeBuilder.cpp Outdated Show resolved Hide resolved
lib/Differentiator/DerivativeBuilder.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
lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a 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

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
@PhrygianGates PhrygianGates force-pushed the operator_overload_rev branch 2 times, most recently from 54b7176 to f45e87a Compare August 30, 2023 10:35
@github-actions
Copy link
Contributor

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

@PhrygianGates PhrygianGates force-pushed the operator_overload_rev branch 2 times, most recently from 75508b2 to dd0bb36 Compare August 30, 2023 12:18
@github-actions
Copy link
Contributor

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

@vgvassilev vgvassilev force-pushed the operator_overload_rev branch from dd0bb36 to 9c5aab1 Compare August 30, 2023 13:09
@github-actions
Copy link
Contributor

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

@vgvassilev vgvassilev force-pushed the operator_overload_rev branch from 9c5aab1 to 514ac7c Compare August 30, 2023 13:25
Copy link
Contributor

@github-actions github-actions bot left a 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

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
@vgvassilev vgvassilev force-pushed the operator_overload_rev branch from 514ac7c to 92d306b Compare August 30, 2023 13:58
Copy link
Contributor

@github-actions github-actions bot left a 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

@@ -1597,8 +1603,11 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context,

/// Add base derivative expression in the derived call output args list if
/// `CE` is a call to an instance member function.
if (auto MCE = dyn_cast<CXXMemberCallExpr>(CE)) {
if (auto MCE = dyn_cast<CXXMemberCallExpr>(CE))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'auto MCE' can be declared as 'const auto *MCE' [llvm-qualified-auto]

Suggested change
if (auto MCE = dyn_cast<CXXMemberCallExpr>(CE))
if (const auto *MCE = dyn_cast<CXXMemberCallExpr>(CE))

@vgvassilev vgvassilev requested a review from parth-07 August 30, 2023 16:50
Copy link
Collaborator

@parth-07 parth-07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need more tests.

  1. Member operator overloads -- operator overloads that are defined inside the class.
  2. Operator overload that does not return a reference type.
  3. Unary operator overload.

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved
@PhrygianGates
Copy link
Contributor Author

PhrygianGates commented Sep 1, 2023

We need more tests.

  1. Member operator overloads -- operator overloads that are defined inside the class.
  2. Operator overload that does not return a reference type.
  3. Unary operator overload.

For 1, our tests already cover the case.
For 2, because we have this prior restriction in the existing code base, we can not support the case yet.

      if (!FD->getReturnType()->isVoidType()) {
        assert((pullback && !FD->getReturnType()->isVoidType()) &&
               "Call to function returning non-void type with no dfdx() is not "
               "supported!");
      }

For 3, I have added tests for unary op.

Copy link
Contributor

@github-actions github-actions bot left a 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

@parth-07
Copy link
Collaborator

parth-07 commented Sep 3, 2023

For 2, because we have this prior restriction in the existing code base, we can not support the case yet.

Why does the restriction that you mentioned prevent non-reference return types? Also, why do you think it only affects non-reference return type operator overloads, but not non-reference return type ordinary member functions?

@parth-07 parth-07 force-pushed the operator_overload_rev branch from 985097e to 578b8fc Compare October 14, 2023 23:26
Copy link
Contributor

@github-actions github-actions bot left a 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

@@ -1749,7 +1762,8 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context,
// derive the called function.
DiffRequest pullbackRequest{};
pullbackRequest.Function = FD;
pullbackRequest.BaseFunctionName = FD->getNameAsString();
pullbackRequest.BaseFunctionName =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: implicit conversion 'clang::Expr *' -> bool [readability-implicit-bool-conversion]

Suggested change
pullbackRequest.BaseFunctionName =
pullbackCallArgs, ArgDeclStmts, dfdx() != nullptr);

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!

@vgvassilev vgvassilev merged commit 9948b21 into vgvassilev:master Oct 15, 2023
77 checks passed
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.

3 participants