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

Reimplement jacobians using the vectorized forward mode #1121

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

PetroZarytskyi
Copy link
Collaborator

Previously, jacobians were based on the non-vectorized reverse mode, which was mostly incapable of capturing multiple outputs. The implementation worked in a few particular cases. For example, it was not possible to differentiate function calls or declare variables inside the original function body.
This PR implements jacobians using the vectorized forward mode. At the very least, this will solve the issues described above and give a way forward to solve other ones. This also means introducing features to the vectorized fwd mode will introduce the same features to jacobians.
Let's take a look at the new signature of jacobians. Since the function to be differentiated is expected to have multiple outputs, we should expect the output in the form of array/pointer/reference parameters (just like before). And for every output parameter, we should generate a corresponding adjoint parameter for the user to acquire the results. Since there is no way to specify which parameters are used as output and which are not, adjoints are generated for all array/pointer/reference parameters. For example:

void f(double a, double b, double* c)  
 --> 
void f_jac(double a, double b, double* c, array_ref<matrix<double>> _d_c)

or

void f(double a, double b, double* c, double[7] t) 
 -->
void f_jac(double a, double b, double* c, double[7] t,
 array_ref<matrix<double>> _d_c, array_ref<matrix<double>> _d_t)

array_ref is necessary for compatibility with the existing infrastructure for vectorized fwd mode overloads generation.
This signature is also similar to the old one. e.g.

df.execute(a, b, c, result); // old behavior
df.execute(a, b, c, &result); // new behavior

However, the behavior differs for multiple output parameters, which the old jacobians did not support.

Note: the same functionality can be achieved by using the vectorized reverse mode, which should probably be implemented at some point. However, the old code for jacobians is unlikely to be useful for that, and there is not much point in keeping it.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 88.26291% with 25 lines in your changes missing coverage. Please review.

Project coverage is 94.03%. Comparing base (d82f7fd) to head (70fcedc).

Files with missing lines Patch % Lines
lib/Differentiator/JacobianModeVisitor.cpp 86.62% 23 Missing ⚠️
lib/Differentiator/BaseForwardModeVisitor.cpp 90.00% 1 Missing ⚠️
lib/Differentiator/VectorForwardModeVisitor.cpp 83.33% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1121      +/-   ##
==========================================
- Coverage   94.37%   94.03%   -0.34%     
==========================================
  Files          50       51       +1     
  Lines        8366     8798     +432     
==========================================
+ Hits         7895     8273     +378     
- Misses        471      525      +54     
Files with missing lines Coverage Δ
lib/Differentiator/DerivativeBuilder.cpp 100.00% <100.00%> (ø)
lib/Differentiator/DiffPlanner.cpp 98.59% <100.00%> (+0.03%) ⬆️
lib/Differentiator/ReverseModeVisitor.cpp 95.06% <100.00%> (-0.45%) ⬇️
lib/Differentiator/VisitorBase.cpp 97.28% <100.00%> (+0.14%) ⬆️
lib/Differentiator/BaseForwardModeVisitor.cpp 98.68% <90.00%> (-0.06%) ⬇️
lib/Differentiator/VectorForwardModeVisitor.cpp 99.69% <83.33%> (-0.31%) ⬇️
lib/Differentiator/JacobianModeVisitor.cpp 86.62% <86.62%> (ø)

... and 27 files with indirect coverage changes

Files with missing lines Coverage Δ
lib/Differentiator/DerivativeBuilder.cpp 100.00% <100.00%> (ø)
lib/Differentiator/DiffPlanner.cpp 98.59% <100.00%> (+0.03%) ⬆️
lib/Differentiator/ReverseModeVisitor.cpp 95.06% <100.00%> (-0.45%) ⬇️
lib/Differentiator/VisitorBase.cpp 97.28% <100.00%> (+0.14%) ⬆️
lib/Differentiator/BaseForwardModeVisitor.cpp 98.68% <90.00%> (-0.06%) ⬇️
lib/Differentiator/VectorForwardModeVisitor.cpp 99.69% <83.33%> (-0.31%) ⬇️
lib/Differentiator/JacobianModeVisitor.cpp 86.62% <86.62%> (ø)

... and 27 files with indirect coverage changes

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

struct JacobianDerivedFnTraits<R (C::*)(Args...) cv vol ref noex> { \
using type = void (C::*)(Args..., SelectLast_t<Args...>) cv vol ref noex; \
};
#define JacobianDerivedFnTraits_AddSPECS(var, cv, vol, ref, noex) \
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function-like macro 'JacobianDerivedFnTraits_AddSPECS' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define JacobianDerivedFnTraits_AddSPECS(var, cv, vol, ref, noex)            \
        ^

clad::utils::ComputeEffectiveFnName(FD) + "_pushforward";
callDiff = m_Builder.BuildCallToCustomDerivativeOrNumericalDiff(
customPushforward, customDerivativeArgs, getCurrentScope(),
const_cast<DeclContext*>(FD->getDeclContext()));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

        const_cast<DeclContext*>(FD->getDeclContext()));
        ^

assert(m_DiffReq.Mode == DiffMode::jacobian);

DiffParams args{};
for (auto dParam : m_DiffReq.DVI)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]

Suggested change
for (auto dParam : m_DiffReq.DVI)
for (const auto& dParam : m_DiffReq.DVI)

// Generate name for the derivative function.
std::string derivedFnName = m_DiffReq.BaseFunctionName + "_jac";
if (args.size() != FD->getNumParams()) {
for (auto arg : args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto arg' can be declared as 'const auto *arg' [llvm-qualified-auto]

Suggested change
for (auto arg : args) {
for (const auto *arg : args) {

std::string derivedFnName = m_DiffReq.BaseFunctionName + "_jac";
if (args.size() != FD->getNumParams()) {
for (auto arg : args) {
auto it = std::find(FD->param_begin(), FD->param_end(), arg);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto it' can be declared as 'const auto *it' [llvm-qualified-auto]

Suggested change
auto it = std::find(FD->param_begin(), FD->param_end(), arg);
const auto *it = std::find(FD->param_begin(), FD->param_end(), arg);

for (size_t i = 0; i < numParamsOriginalFn; ++i) {
bool is_array =
utils::isArrayOrPointerType(m_DiffReq->getParamDecl(i)->getType());
auto param = params[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto param' can be declared as 'auto *param' [llvm-qualified-auto]

Suggested change
auto param = params[i];
auto *param = params[i];


// Traverse the function body and generate the derivative.
Stmt* BodyDiff = Visit(FD->getBody()).getStmt();
if (auto CS = dyn_cast<CompoundStmt>(BodyDiff))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto CS' can be declared as 'auto *CS' [llvm-qualified-auto]

Suggested change
if (auto CS = dyn_cast<CompoundStmt>(BodyDiff))
if (auto *CS = dyn_cast<CompoundStmt>(BodyDiff))

DiffInputVarsInfo DVI;
DVI = request.DVI;
for (auto dParam : DVI)
for (auto dParam : m_DiffReq.DVI)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]

Suggested change
for (auto dParam : m_DiffReq.DVI)
for (const auto& dParam : m_DiffReq.DVI)

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

assert(m_DiffReq.Mode == DiffMode::jacobian);

DiffParams args{};
for (const DiffInputVarInfo& dParam : m_DiffReq.DVI)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]

Suggested change
for (const DiffInputVarInfo& dParam : m_DiffReq.DVI)
for (const auto& dParam : m_DiffReq.DVI)

// Generate name for the derivative function.
std::string derivedFnName = m_DiffReq.BaseFunctionName + "_jac";
if (args.size() != FD->getNumParams()) {
for (const ValueDecl* arg : args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto arg' can be declared as 'const auto *arg' [llvm-qualified-auto]

Suggested change
for (const ValueDecl* arg : args) {
for (const auto *arg : args) {

std::string derivedFnName = m_DiffReq.BaseFunctionName + "_jac";
if (args.size() != FD->getNumParams()) {
for (const ValueDecl* arg : args) {
auto* it = std::find(FD->param_begin(), FD->param_end(), arg);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto it' can be declared as 'const auto *it' [llvm-qualified-auto]

Suggested change
auto* it = std::find(FD->param_begin(), FD->param_end(), arg);
const auto *it = std::find(FD->param_begin(), FD->param_end(), arg);

for (size_t i = 0; i < numParamsOriginalFn; ++i) {
bool is_array =
utils::isArrayOrPointerType(m_DiffReq->getParamDecl(i)->getType());
ParmVarDecl* param = params[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto param' can be declared as 'auto *param' [llvm-qualified-auto]

Suggested change
ParmVarDecl* param = params[i];
auto *param = params[i];


// Traverse the function body and generate the derivative.
Stmt* BodyDiff = Visit(FD->getBody()).getStmt();
if (auto* CS = dyn_cast<CompoundStmt>(BodyDiff))
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto CS' can be declared as 'auto *CS' [llvm-qualified-auto]

Suggested change
if (auto* CS = dyn_cast<CompoundStmt>(BodyDiff))
if (auto *CS = dyn_cast<CompoundStmt>(BodyDiff))

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

std::string derivedFnName = m_DiffReq.BaseFunctionName + "_jac";
if (args.size() != FD->getNumParams()) {
for (const ValueDecl* arg : args) {
auto* it = std::find(FD->param_begin(), FD->param_end(), arg);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'auto *it' can be declared as 'const auto *it' [readability-qualified-auto]

Suggested change
auto* it = std::find(FD->param_begin(), FD->param_end(), arg);
const auto* it = std::find(FD->param_begin(), FD->param_end(), arg);

@vgvassilev
Copy link
Owner

Could we write a few benchmarks to make sure we do not regress in performance?

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