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

Functions with pointer assigns lead to compilation error in reverse mode #197

Closed
grimmmyshini opened this issue Feb 23, 2021 · 6 comments · Fixed by #686
Closed

Functions with pointer assigns lead to compilation error in reverse mode #197

grimmmyshini opened this issue Feb 23, 2021 · 6 comments · Fixed by #686

Comments

@grimmmyshini
Copy link
Contributor

Any assignment of pointers results in a compilation error. Minimum reproducable example:

double f5(double* p) {
  double* another_ptr = p;
  return *another_ptr;
}
clad::gradient(f5);

results in

error: invalid operands to binary expression ('double *' and 'double *')

Ideally, these expressions should be ignored and warned against, they should still be compiled however. I suspect some issue with the _result type mismatch with the intermediate pointers, however, I am not sure. This issue is also closely related to #195 and solving that might also solve this one.

This and #195 both work towards adding array/reference differentiability support too and maybe solved as that feature is addressed.

Another example to reproduce the same error is:

double f5(double a) {
  double* ptr, *another_ptr;
  ptr =  another_ptr;
  return *ptr;
}

results in the same error when executed in reverse mode. It works as expected in the forward mode.

@parth-07
Copy link
Collaborator

Ideally, how should functions containing pointer parameters should be differentiated ?
And differentiating this function in forward mode is giving error too

double f5(double* p) {
  double* another_ptr = p;
  return *another_ptr;
}
int main() {
    clad::differentiate(f5);
}
error: attempted differentiation w.r.t. a parameter ('p') which is not of a real type
double f5(double* p) {

Error obtained using gradient function is not as friendly as the one obtained using differentiation. But why should the code still be compiled ?

@grimmmyshini
Copy link
Contributor Author

Clad does not yet support multi-arg calls (here pointer equivalent) in forward mode. That means while it won't support any non-real argument, it supports pointers being used in the function body as dependent variables/constants. And if we encounter some pointer arithmetic we do not support, we should ideally ignore it and issue a warning stating that generation of a derivative of that operation is not yet supported.

The ideal result of the function causing the issue should just be that it compiles. Maybe there are warnings emitted, but in no case should it result in an error as it does now, especially since the program is syntactically correct. The derivative will not necessarily be correct but that is okay.

As for differentiating function with pointer inputs. The pointers are treated mostly as signifying an array (we don't currently support using pointers with the * notation i.e *(p + 1) = 34 is not supported but p[1] = 34 is supported) and are differentiated accordingly.

@parth-07
Copy link
Collaborator

Thank you for your detailed reply, I understand the situation much better now.
I am having one more doubt, unrelated to this discussion but I am not sure where to ask this, If I am having some doubt/confusion regarding clad codebase, where can I ask about them ?

@grimmmyshini
Copy link
Contributor Author

You can ask here for now, but on that note, @vgvassilev what about a discussions tab on the repo?

@vgvassilev
Copy link
Owner

@parth-07, I would the diagnostics on calling clad::differentiate(f5); a feature ;) We should not use clad::differentiate for array types because clad::gradient is far more efficient. That being said, we might want to do two things to make this clearer:

  • Improve the diagnostics & improve documentation;
  • Allow users to specify that they want to use clad::differentiate on double/float* as they are not array types but just pointers to a single value.clad::differentiate<single_value>(f5). I'd be inclined to support that case only if there is a compelling real world use case for it.

You can open an issue if something is unclear about the codebase. @grimmmyshini I will have to learn more about "Discussions" and maybe that's the longer term answer to that question.

@parth-07
Copy link
Collaborator

@vgvassilev @grimmmyshini I am playing with the clad codebase from last couple of days to understand it better, I am also studying about clang plugins and clang AST, hoping understanding them better will help me to understand the codebase better. I will open an issue if something is unclear, thank you for the supportive responses.

vaithak added a commit to vaithak/clad that referenced this issue Dec 20, 2023
This commit adds support for pointer operation in reverse mode.
The technique is maintain a corresponding derivative pointer
variable, which gets updated (and stored/restored) in the exact same way
as the primal pointer variable in both forward and reverse passes.

Added a workaround (with a FIXME comment) in the UsefulToStoreGlobal
method to essentially bypass TBR analysis results for pointer expr.

Fixes vgvassilev#195, vgvassilev#197
vaithak added a commit to vaithak/clad that referenced this issue Dec 20, 2023
This commit adds support for pointer operation in reverse mode. The technique is to maintain a corresponding derivative pointer variable, which gets updated (and stored/restored) in the exact same way as the primal pointer variable in both forward and reverse passes.
Added a workaround (with a FIXME comment) in the UsefulToStoreGlobal method to essentially bypass TBR analysis results for pointer expr.

Fixes vgvassilev#195, Fixes vgvassilev#197
vaithak added a commit to vaithak/clad that referenced this issue Dec 22, 2023
This commit adds support for pointer operation in reverse mode. The technique is to maintain a corresponding derivative pointer variable, which gets updated (and stored/restored) in the exact same way as the primal pointer variable in both forward and reverse passes.
Added a workaround (with a FIXME comment) in the UsefulToStoreGlobal method to essentially bypass TBR analysis results for pointer expr.

Fixes vgvassilev#195, Fixes vgvassilev#197
vaithak added a commit to vaithak/clad that referenced this issue Dec 29, 2023
This commit adds support for pointer operation in reverse mode. The technique is to maintain a corresponding derivative pointer variable, which gets updated (and stored/restored) in the exact same way as the primal pointer variable in both forward and reverse passes.
Added a workaround (with a FIXME comment) in the UsefulToStoreGlobal method to essentially bypass TBR analysis results for pointer expr.

Fixes vgvassilev#195, Fixes vgvassilev#197
vaithak added a commit to vaithak/clad that referenced this issue Dec 29, 2023
This commit adds support for pointer operation in reverse mode. The technique is to maintain a corresponding derivative pointer variable, which gets updated (and stored/restored) in the exact same way as the primal pointer variable in both forward and reverse passes.
Added a workaround (with a FIXME comment) in the UsefulToStoreGlobal method to essentially bypass TBR analysis results for pointer expr.

Fixes vgvassilev#195, Fixes vgvassilev#197
vaithak added a commit to vaithak/clad that referenced this issue Dec 30, 2023
This commit adds support for pointer operation in reverse mode. The technique is to maintain a corresponding derivative pointer variable, which gets updated (and stored/restored) in the exact same way as the primal pointer variable in both forward and reverse passes.
Added a workaround (with a FIXME comment) in the UsefulToStoreGlobal method to essentially bypass TBR analysis results for pointer expr.

Fixes vgvassilev#195, Fixes vgvassilev#197
vgvassilev pushed a commit that referenced this issue Dec 30, 2023
This commit adds support for pointer operation in reverse mode. The technique is to maintain a corresponding derivative pointer variable, which gets updated (and stored/restored) in the exact same way as the primal pointer variable in both forward and reverse passes.
Added a workaround (with a FIXME comment) in the UsefulToStoreGlobal method to essentially bypass TBR analysis results for pointer expr.

Fixes #195, Fixes #197
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 a pull request may close this issue.

3 participants