Restructure differentiation schedule into a breadth first traversal #848
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was
linked to
issues
Apr 3, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #848 +/- ##
=======================================
Coverage 94.77% 94.77%
=======================================
Files 49 49
Lines 7499 7563 +64
=======================================
+ Hits 7107 7168 +61
- Misses 392 395 +3
|
vaithak
force-pushed
the
diff-plans
branch
2 times, most recently
from
bde43e1 to
8788574
Compare
vaithak
force-pushed
the
diff-plans
branch
3 times, most recently
from
7bfd4aa to
6918417
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
tools/ClangPlugin.h
Outdated
| @@ -288,6 +292,11 @@ namespace clad { | |||
| return P.ProcessDiffRequest(request); | |||
| } | |||
|
|
|||
| void AddRequestToSchedule(CladPlugin& P, | |||
There was a problem hiding this comment.
warning: function 'AddRequestToSchedule' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]
void AddRequestToSchedule(CladPlugin& P,
^Additional context
tools/ClangPlugin.h:294: make as 'inline'
void AddRequestToSchedule(CladPlugin& P,
^
vgvassilev
reviewed
Apr 6, 2024
There was a problem hiding this comment.
Thank you! This is a major improvement. We have discussed that now we can process diff schedules separately from the differentiation process. That is, we first plan and then act. This PR does not get us there just yet, and this is why it relies on a forward declarations because it only appends to the diff plan instead of inserting.
That's an incremental change but we should also try to make the next step maybe in a separate PR.
Comment on lines
+404
to
+407
| if (arg->getName() == "p") | ||
| m_Variables[arg] = m_Result; | ||
| idx += 1; | ||
| continue; |
There was a problem hiding this comment.
Can we come up with a test here?
There was a problem hiding this comment.
This is for Jacobian computation and the particular case is for something related to ROOT as captured here: #478 (comment).
I will look into this when I work on the Jacobain array-related issue: #472
vaithak
force-pushed
the
diff-plans
branch
5 times, most recently
from
d0b651e to
509ec63
Compare
vaithak
force-pushed
the
diff-plans
branch
2 times, most recently
from
a7236d0 to
bfb2c7c
Compare
| CXXScopeSpec SS; | ||
| bool isArrow = Base->getType()->isPointerType(); | ||
| auto ME = m_Sema | ||
| .ActOnMemberAccessExpr(getCurrentScope(), Base, noLoc, | ||
| .ActOnMemberAccessExpr(getCurrentScope(), Base, Loc, |
There was a problem hiding this comment.
warning: 3rd argument 'Loc' (passed to 'OpLoc') looks like it might be swapped with the 6th, 'noLoc' (passed to 'TemplateKWLoc') [readability-suspicious-call-argument]
.ActOnMemberAccessExpr(getCurrentScope(), Base, Loc,
^Additional context
llvm/include/clang/Sema/Sema.h:5307: in the call to 'ActOnMemberAccessExpr', declared here
ExprResult ActOnMemberAccessExpr(Scope *S, Expr *Base,
^There was a problem hiding this comment.
This one seems irrelevant, it is using some heuristic to deduce that noLoc is more similar to OpLoc, maybe because of the Levenshtein distance as mentioned here: https://releases.llvm.org/13.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-suspicious-call-argument.html#levenshtein-distance-as-levenshtein.
vaithak
force-pushed
the
diff-plans
branch
3 times, most recently
from
4f5fabf to
541d126
Compare
vaithak
force-pushed
the
diff-plans
branch
2 times, most recently
from
ba3b914 to
2b7334c
Compare
vgvassilev
reviewed
Apr 11, 2024
vgvassilev
approved these changes
Apr 12, 2024
|
I think the current order of commits is fine. |
vaithak
added a commit
to vaithak/clad
that referenced
this pull request
Apr 30, 2024
Plan for dynamic graph - The relations between different differentiation requests can be modelled as a graph. For example, if `f_a` calls `f_b`, there will be two differentiation requests `df_a` and `df_b`, the edge between them can be understood as `created_because_of`. This also means that the functions called by the users to be explicitly differentiated (or `DiffRequests` created because of these) are the source nodes, i.e. no incoming edges. In most cases, this graph aligns with the call graph, but in some cases, the graph depends on the internal implementation, like the Hessian computation, which requires creating multiple `fwd_mode` requests followed by a `rev_mode` request. - We can use this graph to order the computation of differentiation requests. This was already being done implicitly in the initial recursive implementation. Whenever we encountered a call expression, we started differentiation of the called function; this was sort of like a depth-first search strategy. - This had problems, as `Clang` reported errors when it encountered a new function scope (of the derivative of the called function) in the middle of the old function scope (of the derivative of the callee function). It treated the nested one like a lambda expression. The issue regarding this: vgvassilev#745. - To fix this, an initial strategy was to eliminate the recursive approach. Hence, a queue-based breadth-first approach was implemented in this PR: vgvassilev#848. - Although it fixed the problem, the graph traversal was still implicit. We needed some way to compute/store the complete graph and possibly optimize it, such as converting edges to model the `requires_derivative_of` relation. Using this, we could proceed with differentiation in a topologically sorted ordering. - It also required one caveat: although we don't differentiate the called function completely in a recursive way, we still need to declare it so that we can have the call expression completed (i.e. `auto t0 = f_pushforward(...)`). - To move towards the final stage of having the complete graph computed before starting the differentiation, we need the complete information on how the `DiffRequest` will be formed inside the visitors (including arguments or `DVI` info). This whole approach will require activity analysis in the first pass. - As an incremental improvement, the first requirement was to implement infrastructure to support explicit modelling of the graph and use that to have a breadth-first traversal (and eventually topological ordering). This is the initial PR for capturing the differentiation plan in a graphical format. However, the traversal order is still breadth-first, as we don't have the complete graph in the first pass - mainly because of a lack of information about the args required for `pushforward` and `pullbacks`. This can be improved with the help of activity analysis to capture the complete graph in the first pass, processing the plan in a topologically sorted manner and pruning the graph for user-defined functions. I started this with this approach, and the initial experimental commit is available here for future reference: 82c0b42.
vaithak
added a commit
to vaithak/clad
that referenced
this pull request
May 3, 2024
Plan for dynamic graph - The relations between different differentiation requests can be modelled as a graph. For example, if `f_a` calls `f_b`, there will be two differentiation requests `df_a` and `df_b`, the edge between them can be understood as `created_because_of`. This also means that the functions called by the users to be explicitly differentiated (or `DiffRequests` created because of these) are the source nodes, i.e. no incoming edges. In most cases, this graph aligns with the call graph, but in some cases, the graph depends on the internal implementation, like the Hessian computation, which requires creating multiple `fwd_mode` requests followed by a `rev_mode` request. - We can use this graph to order the computation of differentiation requests. This was already being done implicitly in the initial recursive implementation. Whenever we encountered a call expression, we started differentiation of the called function; this was sort of like a depth-first search strategy. - This had problems, as `Clang` reported errors when it encountered a new function scope (of the derivative of the called function) in the middle of the old function scope (of the derivative of the callee function). It treated the nested one like a lambda expression. The issue regarding this: vgvassilev#745. - To fix this, an initial strategy was to eliminate the recursive approach. Hence, a queue-based breadth-first approach was implemented in this PR: vgvassilev#848. - Although it fixed the problem, the graph traversal was still implicit. We needed some way to compute/store the complete graph and possibly optimize it, such as converting edges to model the `requires_derivative_of` relation. Using this, we could proceed with differentiation in a topologically sorted ordering. - It also required one caveat: although we don't differentiate the called function completely in a recursive way, we still need to declare it so that we can have the call expression completed (i.e. `auto t0 = f_pushforward(...)`). - To move towards the final stage of having the complete graph computed before starting the differentiation, we need the complete information on how the `DiffRequest` will be formed inside the visitors (including arguments or `DVI` info). This whole approach will require activity analysis in the first pass. - As an incremental improvement, the first requirement was to implement infrastructure to support explicit modelling of the graph and use that to have a breadth-first traversal (and eventually topological ordering). This is the initial PR for capturing the differentiation plan in a graphical format. However, the traversal order is still breadth-first, as we don't have the complete graph in the first pass - mainly because of a lack of information about the args required for `pushforward` and `pullbacks`. This can be improved with the help of activity analysis to capture the complete graph in the first pass, processing the plan in a topologically sorted manner and pruning the graph for user-defined functions. I started this with this approach, and the initial experimental commit is available here for future reference: 82c0b42.
vgvassilev
pushed a commit
that referenced
this pull request
May 3, 2024
Plan for dynamic graph - The relations between different differentiation requests can be modelled as a graph. For example, if `f_a` calls `f_b`, there will be two differentiation requests `df_a` and `df_b`, the edge between them can be understood as `created_because_of`. This also means that the functions called by the users to be explicitly differentiated (or `DiffRequests` created because of these) are the source nodes, i.e. no incoming edges. In most cases, this graph aligns with the call graph, but in some cases, the graph depends on the internal implementation, like the Hessian computation, which requires creating multiple `fwd_mode` requests followed by a `rev_mode` request. - We can use this graph to order the computation of differentiation requests. This was already being done implicitly in the initial recursive implementation. Whenever we encountered a call expression, we started differentiation of the called function; this was sort of like a depth-first search strategy. - This had problems, as `Clang` reported errors when it encountered a new function scope (of the derivative of the called function) in the middle of the old function scope (of the derivative of the callee function). It treated the nested one like a lambda expression. The issue regarding this: #745. - To fix this, an initial strategy was to eliminate the recursive approach. Hence, a queue-based breadth-first approach was implemented in this PR: #848. - Although it fixed the problem, the graph traversal was still implicit. We needed some way to compute/store the complete graph and possibly optimize it, such as converting edges to model the `requires_derivative_of` relation. Using this, we could proceed with differentiation in a topologically sorted ordering. - It also required one caveat: although we don't differentiate the called function completely in a recursive way, we still need to declare it so that we can have the call expression completed (i.e. `auto t0 = f_pushforward(...)`). - To move towards the final stage of having the complete graph computed before starting the differentiation, we need the complete information on how the `DiffRequest` will be formed inside the visitors (including arguments or `DVI` info). This whole approach will require activity analysis in the first pass. - As an incremental improvement, the first requirement was to implement infrastructure to support explicit modelling of the graph and use that to have a breadth-first traversal (and eventually topological ordering). This is the initial PR for capturing the differentiation plan in a graphical format. However, the traversal order is still breadth-first, as we don't have the complete graph in the first pass - mainly because of a lack of information about the args required for `pushforward` and `pullbacks`. This can be improved with the help of activity analysis to capture the complete graph in the first pass, processing the plan in a topologically sorted manner and pruning the graph for user-defined functions. I started this with this approach, and the initial experimental commit is available here for future reference: vaithak@82c0b42.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.