Enable recursive differentiation of nested calls #117
Conversation
test/FirstDerivative/Recursive.C
Outdated
| //cCHECK-NEXT: int _t0 = f_pow(arg, p - 1); | ||
| //cCHECK-NEXT: return _d_arg * _t0 + arg * (f_pow_darg0(arg, p - 1) * (_d_arg + _d_p - 0)); | ||
| //cCHECK-NEXT: } | ||
| //cCHECK-NEXT: } |
There was a problem hiding this comment.
This test is broken atm and was disabled.
There was a problem hiding this comment.
Could you prepend FIXME- and open an issue for this?
There was a problem hiding this comment.
OK, the issue is #116
| //cCHECK-NEXT: _d_x = _d_x * _t1 + x * (custom_derivatives::pow_darg0(2., y) * (0. + _d_y)); | ||
| //cCHECK-NEXT: x *= _t1; | ||
| //cCHECK-NEXT: return _d_x; | ||
| //cCHECK-NEXT: } |
There was a problem hiding this comment.
This test is broken atm and was disabled.
efremale
force-pushed
the
nested-calls
branch
3 times, most recently
from
2891c31
to
05e83ac
Compare
| /// Name of the base function to be differentiated. Can be different from | ||
| /// function->getNameAsString() when higher-order derivatives are computed. | ||
| std::string baseFunctionName = {}; | ||
| /// Current derivative order to be computer. |
There was a problem hiding this comment.
| /// Current derivative order to be computer. | |
| /// Current derivative order to be computed. |
| @@ -424,9 +435,10 @@ namespace clad { | |||
| NamespaceDecl* enclosingNS = nullptr; | |||
| llvm::SaveAndRestore<DeclContext*> SaveContext(m_Sema.CurContext); | |||
| llvm::SaveAndRestore<Scope*> SaveScope(m_CurScope); | |||
| m_Sema.CurContext = m_Function->getDeclContext(); | |||
| DeclContext* DC = const_cast<DeclContext*>(m_Function->getDeclContext()); | |||
There was a problem hiding this comment.
We should make m_Function non const.
There was a problem hiding this comment.
It is not really a solution, since we are assigning FD (const FunctionDecl* passed to ::Derive, which is indeed intended to be const, since we are not going to modify the original declaration) to m_Function. So that will just move the const_cast elsewhere.
It is more reasonable to remove constness from the DeclContext since we are indeed going to modify it (e.g. by putting new derivative into the original function's namespace).
| @@ -1339,9 +1315,10 @@ namespace clad { | |||
| NamespaceDecl* enclosingNS = nullptr; | |||
| llvm::SaveAndRestore<DeclContext*> SaveContext(m_Sema.CurContext); | |||
| llvm::SaveAndRestore<Scope*> SaveScope(m_CurScope); | |||
| m_Sema.CurContext = m_Function->getDeclContext(); | |||
| DeclContext* DC = const_cast<DeclContext*>(m_Function->getDeclContext()); | |||
| @@ -96,7 +96,7 @@ float f_const_args_func_6(const float x, const float y, const Vec &v) { | |||
|
|
|||
| float f_const_helper(const float x) { | |||
| return x * x; | |||
| } // expected-warning 4 {{function 'f_const_helper' was not differentiated because it is not declared in namespace 'custom_derivatives'}} | |||
| } | |||
| int f_1(int x) { | ||
| printf("I am being run!\n"); //expected-warning{{attempted to differentiate unsupported statement, no changes applied}} | ||
| printf("I am being run!\n"); //expected-warning{{attempted to differentiate unsupported statement, no changes applied}} //expected-warning{{function 'printf' was not differentiated because clad failed to differentiate it and no suitable overload was found in namespace 'custom_derivatives'}} |
There was a problem hiding this comment.
We should shorten the diagnostic and make it more accurate. What about “function ... was not differentiated because no definition is available and no suitable replacement ...”?
There was a problem hiding this comment.
It is not necessarily that there is no definition. There may be a definition but clad may fail to derive it (e.g. if it meets something it doesn't support).
There was a problem hiding this comment.
I think In that case we should diagnose that in a separate diagnostic.
There was a problem hiding this comment.
That is not so easy to do at the moment and we'll need to work on some diagnostic infrastructure to propagate exact and clear errors from recursive calls to ::Derive.
There was a problem hiding this comment.
Ok, could you open an issue for that then?
| /// A struct containing information about request to differentiate a function. | ||
| struct DiffRequest { | ||
| /// Function to be differentiated. | ||
| const clang::FunctionDecl* function = nullptr; |
There was a problem hiding this comment.
We should probably either add m_ to all members or at least capitalize them.
There was a problem hiding this comment.
What is wrong with request.function? It is the cleanest way IMO. It is just a struct carrying parameters.
There was a problem hiding this comment.
We seem to be using different pattern elsewhere.
There was a problem hiding this comment.
It is questionable why are we using m_ thing in the first place. Maybe it makes some sense in classes where only a small part is exposed as a public interface. It doesn't make a lot of sense to use it on a config struct where members are intended to be assigned with something as a public interface.
There was a problem hiding this comment.
We should have accessors but that'd be an overkill for that. Let's use llvm's convention capitalizing the data members.
There was a problem hiding this comment.
Accessors like request.setCurrentDerivativeOrder(request.getCurrentDerivativeOrder() + 1)? I don't think we should ever have that.
Now, if a function `f` that is being differentiated contains a call to a function `g`, clad will proceed to try to differentiate `g` even if it has no
overload in `custom_derivatives`.
Example:
```
double f(double x) {
return g(double x);
}
```
On `clad::differentiate(f, 0)`, clad will proceed to differentiate `g` and so on, even if it is not in `custom_derivatives` namespace.
Now, if a function
fthat is being differentiated contains a call to a functiong, clad will proceed to try to differentiategeven if it has nooverload in
custom_derivatives.Example:
On
clad::differentiate(f, 0), clad will proceed to differentiategand so on, even if it is not incustom_derivativesnamespace.A new issue with forward mode calls was detected: #116, we should fix it. Some tests were disabled.