Delay the differentiation process until the end of TU. #766
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #766 +/- ##
==========================================
+ Coverage 94.86% 94.97% +0.11%
==========================================
Files 49 49
Lines 7357 7543 +186
==========================================
+ Hits 6979 7164 +185
- Misses 378 379 +1
|
vgvassilev
force-pushed
the
issue-248
branch
2 times, most recently
from
b479740 to
55c80ca
Compare
vgvassilev
force-pushed
the
issue-248
branch
from
55c80ca to
18a17ac
Compare
vgvassilev
force-pushed
the
issue-248
branch
2 times, most recently
from
39341b1 to
07723ff
Compare
vgvassilev
force-pushed
the
issue-248
branch
from
07723ff to
df334e9
Compare
vgvassilev
force-pushed
the
issue-248
branch
5 times, most recently
from
79b9564 to
7cbbb0f
Compare
| @@ -76,13 +76,13 @@ int main() { | |||
| // the indexes of the array by using the format arr[0:<last index of arr>] | |||
| auto hessian_all = clad::hessian(weighted_avg, "arr[0:2], weights[0:2]"); | |||
| // Generates the Hessian matrix for weighted_avg w.r.t. to arr. | |||
| auto hessian_arr = clad::hessian(weighted_avg, "arr[0:2]"); | |||
| // auto hessian_arr = clad::hessian(weighted_avg, "arr[0:2]"); | |||
There was a problem hiding this comment.
I am not particularly happy with this commit, however, this is the only way to make progress here. I believe the demo fails due to the fact we schedule the hessians recursively. This seems hard to debug because it happens on clang release builds and clad for runtime11, runtime10, and in some cases runtime9.
More information: https://github.com/vgvassilev/clad/actions/runs/8216311966
Perhaps once we fix the way we order diff plans the issue will go away.
vgvassilev
force-pushed
the
issue-248
branch
2 times, most recently
from
dc91959 to
794d579
Compare
vgvassilev
force-pushed
the
issue-248
branch
4 times, most recently
from
7fc3293 to
481e013
Compare
tools/ClangPlugin.h
Outdated
|
|
||
| // FIXME: We should replace this comparison with the canonical decl | ||
| // from the differentiation plan... | ||
| return ND->getName().contains("_darg"); |
There was a problem hiding this comment.
We need to ignore directly referenced _grad, _hessian and _jacobian calls as well. Perhaps we should discourage users from directly calling the generated functions. This way, we would not need to ignore the warning.
There was a problem hiding this comment.
We need to ignore directly referenced _grad, _hessian and _jacobian calls as well.
Added.
Perhaps we should discourage users from directly calling the generated functions.
I thought about the same but I hesitate because this way we can do cross translation unit derivative communication because that's what the language does for everything else.
This way, we would not need to ignore the warning.
This warning triggers only when we put a forward declaration in a anonymous namespace. That's quite silly thing to do generally but...
There was a problem hiding this comment.
I thought about the same but I hesitate because this way we can do cross translation unit derivative communication because that's what the language does for everything else.
Oh, yes, these function declarations can be very helpful for cross-translation derivative communication. We should definitely document about this in the documentation and how users can depend upon and use this functionality.
There was a problem hiding this comment.
It would be helpful to add an interface which users can easily use for determining the derivative type of a given function. Typing the derivative function type by hand can be error-prone and cumbersome
There was a problem hiding this comment.
Yes, I am not sure how. If you have an idea maybe we should open an issue?
|
Should we add a detailed description explaining these changes. It would be a helpful reference for us and even new contributors to understand the request flow. |
Do you mean in the commit message or the clad documentation? |
In the commit message should be sufficient for now, once we have the diff plans also finalized, we can add them in the documentation too if required. |
I thought I capture that in the very first commit message: What else is missing? |
|
My bad, didn't notice it was in the commit message itself. This should be fine. |
vgvassilev
force-pushed
the
issue-248
branch
3 times, most recently
from
4735b41 to
51c8df5
Compare
vgvassilev
force-pushed
the
issue-248
branch
2 times, most recently
from
66cde27 to
0d89cdd
Compare
Before this patch clad attaches itself as a first consumer and applies AD before code generation. However, that is limited since clang sends every top-level declaration to codegen which limits the amount of flexibility clad has. For example, we have to instantiate all pending templates at every HandleTopLevelDecl calls; we cannot really differentiate virtual functions whose classes have sent their key function to CodeGen; and in general we perform actions which are semantically useful for the end of the translation unit. This patch makes clad a single consumer of clang which dispatches to the others. That's done by delaying all calls to the consumers until the end of the TU where clad can replay the exact sequence of calls to the other consumers as if they were directly connected to the frontend. Fixes #248
We should be able to diagnose these problems as reported in #810.
The demo fails due to the fact we schedule the hessians recursively. This seems hard to debug because it happens on clang release builds and clad for runtime11, runtime10, and in some cases runtime9. More information: https://github.com/vgvassilev/clad/actions/runs/8216311966
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| DelayedCallInfo(CallKind K, clang::DeclGroupRef DGR) | ||
| : m_Kind(K), m_DGR(DGR) {} | ||
| DelayedCallInfo(CallKind K, const clang::Decl* D) | ||
| : m_Kind(K), m_DGR(const_cast<clang::Decl*>(D)) {} |
There was a problem hiding this comment.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
: m_Kind(K), m_DGR(const_cast<clang::Decl*>(D)) {}
^
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This patch enhances the work done in #766 where we assumed the clients are going to be well-behaved such as clang or clang-repl. However, we can have clients which connect the clang components in a non-standard way and we should provide a reasonable fallback. This patch detects if the setup is non-standard and teaches clad to handle requests as they come, pretty much the same way as before #766 was implemented. Of course this comes with a cost when we try to differentiate declarations that are defined later in the translation unit. We expect such setups to be rare and mostly in the cases of incremental processing and old repls such as Cling.
This patch enhances the work done in #766 where we assumed the clients are going to be well-behaved such as clang or clang-repl. However, we can have clients which connect the clang components in a non-standard way and we should provide a reasonable fallback. This patch detects if the setup is non-standard and teaches clad to handle requests as they come, pretty much the same way as before #766 was implemented. Of course this comes with a cost when we try to differentiate declarations that are defined later in the translation unit. We expect such setups to be rare and mostly in the cases of incremental processing and old repls such as Cling.
This patch enhances the work done in #766 where we assumed the clients are going to be well-behaved such as clang or clang-repl. However, we can have clients which connect the clang components in a non-standard way and we should provide a reasonable fallback. This patch detects if the setup is non-standard and teaches clad to handle requests as they come, pretty much the same way as before #766 was implemented. Of course this comes with a cost when we try to differentiate declarations that are defined later in the translation unit. We expect such setups to be rare and mostly in the cases of incremental processing and old repls such as Cling.
This patch enhances the work done in #766 where we assumed the clients are going to be well-behaved such as clang or clang-repl. However, we can have clients which connect the clang components in a non-standard way and we should provide a reasonable fallback. This patch detects if the setup is non-standard and teaches clad to handle requests as they come, pretty much the same way as before #766 was implemented. Of course this comes with a cost when we try to differentiate declarations that are defined later in the translation unit. We expect such setups to be rare and mostly in the cases of incremental processing and old repls such as Cling.
Before this patch clad attaches itself as a first consumer and applies AD before code generation. However, that is limited since clang sends every top-level declaration to codegen which limits the amount of flexibility clad has. For example, we have to instantiate all pending templates at every HandleTopLevelDecl calls; we cannot really differentiate virtual functions whose classes have sent their key function to CodeGen; and in general we perform actions which are semantically useful for the end of the translation unit.
This patch makes clad a single consumer of clang which dispatches to the others. That's done by delaying all calls to the consumers until the end of the TU where clad can replay the exact sequence of calls to the other consumers as if they were directly connected to the frontend.
Fixes #248