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

Add support for simple lambda expressions in forward mode #937

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

gojakuch
Copy link
Collaborator

@gojakuch gojakuch commented Jun 12, 2024

This commit provides support for the simplest lambda expressions, that is, those with no captures, in forward mode. The original lambda function is copied into the derivative, but the corresponding lambda class gets extended to also have a pushforward method for the call operator overload. Essentially, there's no visiting of lambda expressions yet (this will be implemented later to support captures, I'm working on this), but rather we allow treating simple cases of lambda functions as functors.

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

codecov bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.80%. Comparing base (de8a6f6) to head (3161ee1).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #937   +/-   ##
=======================================
  Coverage   93.79%   93.80%           
=======================================
  Files          55       55           
  Lines        7815     7827   +12     
=======================================
+ Hits         7330     7342   +12     
  Misses        485      485           
Files Coverage Δ
lib/Differentiator/BaseForwardModeVisitor.cpp 98.94% <100.00%> (+0.01%) ⬆️
Files Coverage Δ
lib/Differentiator/BaseForwardModeVisitor.cpp 98.94% <100.00%> (+0.01%) ⬆️

@gojakuch gojakuch marked this pull request as draft June 13, 2024 08:45
@gojakuch gojakuch marked this pull request as ready for review June 13, 2024 15:55
Copy link
Contributor

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

baseDerivative = BuildOp(UnaryOperatorKind::UO_AddrOf, baseDerivative);
diffArgs.push_back(baseDerivative);
if (const auto* MD = dyn_cast<CXXMethodDecl>(FD)) {
if (!isLambda) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd invert the condition to reduce the negations. Also, we might just use clang::isLambdaCallOperator for this check in the if-stmt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

error: ‘isLambdaCallOperator’ was not declared in this scope.

I couldn't manage to use this check unfortunately. but I inverted the condition

Copy link
Owner

Choose a reason for hiding this comment

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

Did you include clang/AST/ASTLambda.h before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, for some reason I was sure that it's included. it finds the method now but if I set isLambda variable to isLambdaCallOperator(FD->getDeclContext()) on the line 1035 (which is, I assume, is how it's supposed to work) it's just not equivalent to the current check so it actually crashes:

// current check, equals to 1 in lambda tests
isLambda = (FD->getDeclContext()->isRecord() && FD->getDeclContext()->getOuterLexicalRecordContext()->isLambda());
// new check, equals to 0 in lambda tests
isLambda = isLambdaCallOperator(FD->getDeclContext());

I'm might be using isLambdaCallOperator wrong.

btw I suggest that isLambda is kept as a variable if you meant that we should call isLambdaCallOperator in the if-condition directly because it's used twice already and might be used more as I implement this further.

Copy link
Owner

Choose a reason for hiding this comment

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

My suggestion to use a "standard" check was to make sure we do what the compiler does to check if something is a lambda. Why does it fail?

Copy link
Owner

Choose a reason for hiding this comment

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

isLambdaCallOperator takes a CXXMethodDecl. You are taking the parent DeclContext which is the class...

@gojakuch gojakuch force-pushed the lambda-support branch 2 times, most recently from b286af9 to 9dd25bf Compare June 14, 2024 18:15
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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

SourceLocation validLoc{CE->getBeginLoc()};

// Calls to lambda functions are processed differently
bool isLambda = MD && isLambdaCallOperator(MD);
Copy link
Owner

Choose a reason for hiding this comment

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

This should be able to take FD as is. Can you pass that instead without casting it?

SourceLocation validLoc{CE->getBeginLoc()};

// Calls to lambda functions are processed differently
bool isLambda = MD && isLambdaCallOperator(MD);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
bool isLambda = MD && isLambdaCallOperator(MD);
bool isLambda = isLambdaCallOperator(FD);

@@ -1036,9 +1037,12 @@ StmtDiff BaseForwardModeVisitor::VisitCallExpr(const CallExpr* CE) {
"Differentiation of only direct calls is supported. Ignored");
return StmtDiff(Clone(CE));
}

const auto* MD = dyn_cast<CXXMethodDecl>(FD);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const auto* MD = dyn_cast<CXXMethodDecl>(FD);

This commit provides support for the simplest lambda functions,
that is, those with no captures in forward mode. The original
lambda function is copied into the derivative, but the
corresponding lambda class gets extended to also have a pushforward
method for the call operator overload.
Copy link
Contributor

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

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 00a3fb8 into vgvassilev:master Jun 17, 2024
89 checks passed
gojakuch added a commit to gojakuch/clad that referenced this pull request Jun 21, 2024
This commit provides support for primitive lambda expressions
with no captures in reverse mode in the same way they are
currently supported in the forward mode (vgvassilev#937). That is, the
lambda expressions are not visited yet. Instead, the lambda
functions are treated as a special case of functors.

Fixes: vgvassilev#789
gojakuch added a commit to gojakuch/clad that referenced this pull request Jun 22, 2024
This commit provides support for primitive lambda expressions
with no captures in reverse mode in the same way they are
currently supported in the forward mode (vgvassilev#937). That is, the
lambda expressions are not visited yet. Instead, the lambda
functions are treated as a special case of functors.

Fixes: vgvassilev#789
gojakuch added a commit to gojakuch/clad that referenced this pull request Jun 22, 2024
This commit provides support for primitive lambda expressions
with no captures in reverse mode in the same way they are
currently supported in the forward mode (vgvassilev#937). That is, the
lambda expressions are not visited yet. Instead, the lambda
functions are treated as a special case of functors.

Fixes: vgvassilev#789
gojakuch added a commit to gojakuch/clad that referenced this pull request Jun 22, 2024
This commit provides support for primitive lambda expressions
with no captures in reverse mode in the same way they are
currently supported in the forward mode (vgvassilev#937). That is, the
lambda expressions are not visited yet. Instead, the lambda
functions are treated as a special case of functors.

Fixes: vgvassilev#789
gojakuch added a commit to gojakuch/clad that referenced this pull request Jun 23, 2024
This commit provides support for primitive lambda expressions
with no captures in reverse mode in the same way they are
currently supported in the forward mode (vgvassilev#937). That is, the
lambda expressions are not visited yet. Instead, the lambda
functions are treated as a special case of functors.

Fixes: vgvassilev#789
gojakuch added a commit to gojakuch/clad that referenced this pull request Jun 23, 2024
This commit provides support for primitive lambda expressions
with no captures in reverse mode in the same way they are
currently supported in the forward mode (vgvassilev#937). That is, the
lambda expressions are not visited yet. Instead, the lambda
functions are treated as a special case of functors.

Fixes: vgvassilev#789
vgvassilev pushed a commit to gojakuch/clad that referenced this pull request Jul 1, 2024
This commit provides support for primitive lambda expressions
with no captures in reverse mode in the same way they are
currently supported in the forward mode (vgvassilev#937). That is, the
lambda expressions are not visited yet. Instead, the lambda
functions are treated as a special case of functors.

Fixes: vgvassilev#789
vgvassilev pushed a commit that referenced this pull request Jul 1, 2024
This commit provides support for primitive lambda expressions
with no captures in reverse mode in the same way they are
currently supported in the forward mode (#937). That is, the
lambda expressions are not visited yet. Instead, the lambda
functions are treated as a special case of functors.

Fixes: #789
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.

2 participants