fix: use adjoint while move it by fogsong233 · Pull Request #1772 · vgvassilev/clad

fogsong233 · 2026-03-12T16:47:11Z

I have currently implemented a fix with maximum compatibility by creating a copy. However, xvalues are still being used within the function; this incorrect usage of rvalues is a systemic issue throughout the entire workflow.

It appears that Clad is not correctly perceiving the move operation, which prevents the proper transformation of the adjoint. This suggests a potential design conflict between the semantics of a 'move' and the requirements of automatic differentiation

github-actions · 2026-03-12T16:56:35Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2026-03-12T17:18:26Z

lib/Differentiator/ReverseModeVisitor.cpp

    if (argDiff.getExpr_dx() && arg->isXValue() &&
        argDiff.getExpr_dx()->isLValue()) {
-      llvm::SmallVector<Expr*, 1> moveArg = {argDiff.getExpr_dx()};
+      Expr* revSweepAdjoint = Clone(argDiff.getExpr_dx());


We will need a test for this.

Maybe we should also need change the exsiting test,which checks generated code as before

fogsong233 · 2026-03-13T17:29:58Z

#include <clad/Differentiator/CladtorchBuiltins.h>
#include <clad/Differentiator/Differentiator.h>

using FTensor = cladtorch::Tensor<float>;

struct M {
  FTensor f(const FTensor& a, const FTensor& b) const {
    auto t = a + b;
    return t;
  }
};

float loss(const M& m, const FTensor& a, const FTensor& b, const FTensor& c) {
  auto y = m.f(a, b);
  auto z = y * c;
  auto n = z.norm();
  return n.scalar();
}

int main() {
  auto grad = clad::gradient(loss, "1");
  (void)grad;
  return 0;
}

See this example: after clad's process:

clad::ValueAndAdjoint<FTensor, FTensor> f_reverse_forw(const FTensor &a, const FTensor &b, const M *_d_this, const FTensor &_d_a, const FTensor &_d_b) const {
    ::clad::ValueAndAdjoint< ::cladtorch::Tensor<float>, ::cladtorch::Tensor<float> > _t0 = clad::custom_derivatives::class_functions::operator_plus_reverse_forw(&a, b, &_d_a, _d_b);
    cladtorch::Tensor<float> t = _t0.value;
    cladtorch::Tensor<float> _d_t = _t0.adjoint;
    ::clad::ValueAndAdjoint< ::cladtorch::Tensor<float>, ::cladtorch::Tensor<float> > _t1 = clad::custom_derivatives::class_functions::constructor_reverse_forw(clad::Tag<Tensor<float> >(), std::move(t), std::move(_d_t));
    return {_t1.value, _t1.adjoint};
}
void f_pullback(const FTensor &a, const FTensor &b, FTensor _d_y, M *_d_this, FTensor *_d_a, FTensor *_d_b) const {
    ::clad::ValueAndAdjoint< ::cladtorch::Tensor<float>, ::cladtorch::Tensor<float> > _t0 = clad::custom_derivatives::class_functions::operator_plus_reverse_forw(&a, b, _d_a, (*_d_b));
    cladtorch::Tensor<float> t = _t0.value;
    cladtorch::Tensor<float> _d_t = _t0.adjoint;
    ::clad::ValueAndAdjoint< ::cladtorch::Tensor<float>, ::cladtorch::Tensor<float> > _t1 = clad::custom_derivatives::class_functions::constructor_reverse_forw(clad::Tag<Tensor<float> >(), std::move(t), std::move(_d_t));
    clad::custom_derivatives::class_functions::constructor_pullback(std::move(t), &_d_y, &_d_t);
    clad::custom_derivatives::class_functions::operator_plus_pullback(&a, b, _d_t, _d_a, _d_b);
}
void loss_grad_1(const M &m, const FTensor &a, const FTensor &b, const FTensor &c, FTensor *_d_a) {
    M _d_m = {};
    FTensor _d_b(b);
    FTensor _d_c(c);
    clad::ValueAndAdjoint<FTensor, FTensor> _t0 = m.f_reverse_forw(a, b, &_d_m, (*_d_a), _d_b);
    cladtorch::FTensor y = _t0.value;
    cladtorch::FTensor _d_y = _t0.adjoint;
    ::clad::ValueAndAdjoint< ::cladtorch::Tensor<float>, ::cladtorch::Tensor<float> > _t1 = clad::custom_derivatives::class_functions::operator_star_reverse_forw(&y, c, &_d_y, _d_c);
    cladtorch::Tensor<float> z = _t1.value;
    cladtorch::Tensor<float> _d_z = _t1.adjoint;
    ::clad::ValueAndAdjoint< ::cladtorch::Tensor<float>, ::cladtorch::Tensor<float> > _t2 = clad::custom_derivatives::class_functions::norm_reverse_forw(&z, &_d_z);
    cladtorch::Tensor<float> n = _t2.value;
    cladtorch::Tensor<float> _d_n = _t2.adjoint;
    clad::custom_derivatives::class_functions::scalar_pullback(&n, 1, &_d_n);
    clad::custom_derivatives::class_functions::norm_pullback(&z, _d_n, &_d_z);
    clad::custom_derivatives::class_functions::operator_star_pullback(&y, c, _d_z, &_d_y, &_d_c);
    m.f_pullback(a, b, _d_y, &_d_m, _d_a, &_d_b);
}

There:

  cladtorch::Tensor<float> _d_t = _t0.adjoint;
    ::clad::ValueAndAdjoint< ::cladtorch::Tensor<float>, ::cladtorch::Tensor<float> > _t1 = clad::custom_derivatives::class_functions::constructor_reverse_forw(clad::Tag<Tensor<float> >(), std::move(t), std::move(_d_t));
    clad::custom_derivatives::class_functions::constructor_pullback(std::move(t), &_d_y, &_d_t);
    clad::custom_derivatives::class_functions::operator_plus_pullback(&a, b, _d_t, _d_a, _d_b);

It use after move

vgvassilev · 2026-03-17T07:28:15Z

@guitargeek, I lack bandwidth but can you take a look?

guitargeek · 2026-03-17T10:16:04Z

Sure! Can @fogsong233, could you first make sure that the CI passes? Also, does this PR address an existing GitHub issue?

guitargeek · 2026-03-17T10:33:15Z

In particular, there are merge conflicts that need to be resolved.

guitargeek

Needs rebase

fogsong233 · 2026-03-17T15:39:12Z

Sure! Can @fogsong233, could you first make sure that the CI passes? Also, does this PR address an existing GitHub issue?

This PR changes the structure of the generated code, but the plan hasn’t been finalized yet. I think we could first implement a solution, and then update the CI to match the new structure once it’s properly defined.
Do you have some idea to fix it? I'm not very familiar with the pipeline of reverse mode visitor.

fogsong233 · 2026-03-17T15:39:49Z

Sure! Can @fogsong233, could you first make sure that the CI passes? Also, does this PR address an existing GitHub issue?

This is a new problem and not related to any existing issue.

vgvassilev · 2026-03-21T16:00:53Z

Sure! Can @fogsong233, could you first make sure that the CI passes? Also, does this PR address an existing GitHub issue?

This is a new problem and not related to any existing issue.

Can we bisect it? Maybe somewhere there was a faulty merge.

github-actions · 2026-03-21T16:12:55Z

clang-tidy review says "All clean, LGTM! 👍"

fogsong233 · 2026-03-22T10:23:02Z

Sure! Can @fogsong233, could you first make sure that the CI passes? Also, does this PR address an existing GitHub issue?

This is a new problem and not related to any existing issue.

Can we bisect it? Maybe somewhere there was a faulty merge.

Okay, i will try to find it, but i guess it is the commit that impl the move-specific path in reverse mode that causes it.

vgvassilev · 2026-03-23T16:32:18Z

Sure! Can @fogsong233, could you first make sure that the CI passes? Also, does this PR address an existing GitHub issue?

This is a new problem and not related to any existing issue.

Can we bisect it? Maybe somewhere there was a faulty merge.

Okay, i will try to find it, but i guess it is the commit that impl the move-specific path in reverse mode that causes it.

Perhaps, but let's make sure that's the case.

fogsong233 · 2026-03-24T12:28:24Z

Sure! Can @fogsong233, could you first make sure that the CI passes? Also, does this PR address an existing GitHub issue?

This is a new problem and not related to any existing issue.

Can we bisect it? Maybe somewhere there was a faulty merge.

9ed0b9e Generate constructor_reverse_forw automatically

Sure! Can @fogsong233, could you first make sure that the CI passes? Also, does this PR address an existing GitHub issue?

This is a new problem and not related to any existing issue.

Can we bisect it? Maybe somewhere there was a faulty merge.

Okay, i will try to find it, but i guess it is the commit that impl the move-specific path in reverse mode that causes it.

Perhaps, but let's make sure that's the case.

#1625
It seems that this pr makes this happens.

vgvassilev · 2026-03-24T19:02:44Z

Ok, then we will need a fix to move forward. Eg. we need to resolve this as either part of this PR or a separate PR.

github-actions · 2026-03-24T19:14:52Z

clang-tidy review says "All clean, LGTM! 👍"

fogsong233 · 2026-03-26T16:33:32Z

Ok, then we will need a fix to move forward. Eg. we need to resolve this as either part of this PR or a separate PR.

I currently see two possible ways to address this issue, but neither is fully satisfactory because move construction is special.

Method 1:
Fall back to the pre-PR behavior for copy/move constructors, i.e. do not build or use constructor_reverse_forw at the call site.

This is the simplest and safest fix, and it avoids the immediate use-after-move problem. However, it also means we give up the new constructor_reverse_forw path for copy/move constructors, so any user-defined special behavior encoded there would no longer be used in this case.

Method 2:
Try to continue from the result produced by the first move, instead of reusing the original source object for the later differentiation steps.

At first glance this looks attractive, but I do not think it is generally sound. A move constructor is not just an ordinary pure function call: it may consume or transform the entire source object. In reverse mode, the current pipeline may need to touch the same construction in multiple stages (constructor_reverse_forw, forward reconstruction, and constructor_pullback). If we reuse the moved-to object as if it were the original source, this may be incorrect for user-defined move constructors.

For example:

struct S {
  double x;
  S(S&& o) : x(o.x * o.x) { o.x = 0; }
};

Here the destination object stores transformed state, not the original pre-move source state. So using the moved-to object for pullback is not generally equivalent to replaying the original move from the original source.

Therefore, i prefer the first method, i think it is enough, simple and good. Because move constructor itself should be something just move the data, not some heavy behavior, i think constructor_reverse_forw is useless most of time.

vgvassilev · 2026-03-26T16:49:16Z

Ok, then we will need a fix to move forward. Eg. we need to resolve this as either part of this PR or a separate PR.

I currently see two possible ways to address this issue, but neither is fully satisfactory because move construction is special.

Method 1: Fall back to the pre-PR behavior for copy/move constructors, i.e. do not build or use constructor_reverse_forw at the call site.

This is the simplest and safest fix, and it avoids the immediate use-after-move problem. However, it also means we give up the new constructor_reverse_forw path for copy/move constructors, so any user-defined special behavior encoded there would no longer be used in this case.

Method 2: Try to continue from the result produced by the first move, instead of reusing the original source object for the later differentiation steps.

At first glance this looks attractive, but I do not think it is generally sound. A move constructor is not just an ordinary pure function call: it may consume or transform the entire source object. In reverse mode, the current pipeline may need to touch the same construction in multiple stages (constructor_reverse_forw, forward reconstruction, and constructor_pullback). If we reuse the moved-to object as if it were the original source, this may be incorrect for user-defined move constructors.

For example:
struct S {
  double x;
  S(S&& o) : x(o.x * o.x) { o.x = 0; }
};
Here the destination object stores transformed state, not the original pre-move source state. So using the moved-to object for pullback is not generally equivalent to replaying the original move from the original source.

Therefore, i prefer the first method, i think it is enough, simple and good. Because move constructor itself should be something just move the data, not some heavy behavior, i think constructor_reverse_forw is useless most of time.

I'd opt for Method1 + opening a new issue to track this discussion and explain that we need a better fix.

fogsong233 · 2026-03-26T16:49:49Z

Ok, then we will need a fix to move forward. Eg. we need to resolve this as either part of this PR or a separate PR.

I currently see two possible ways to address this issue, but neither is fully satisfactory because move construction is special.
Method 1: Fall back to the pre-PR behavior for copy/move constructors, i.e. do not build or use constructor_reverse_forw at the call site.
This is the simplest and safest fix, and it avoids the immediate use-after-move problem. However, it also means we give up the new constructor_reverse_forw path for copy/move constructors, so any user-defined special behavior encoded there would no longer be used in this case.
Method 2: Try to continue from the result produced by the first move, instead of reusing the original source object for the later differentiation steps.
At first glance this looks attractive, but I do not think it is generally sound. A move constructor is not just an ordinary pure function call: it may consume or transform the entire source object. In reverse mode, the current pipeline may need to touch the same construction in multiple stages (constructor_reverse_forw, forward reconstruction, and constructor_pullback). If we reuse the moved-to object as if it were the original source, this may be incorrect for user-defined move constructors.
For example:
struct S {
  double x;
  S(S&& o) : x(o.x * o.x) { o.x = 0; }
};
Here the destination object stores transformed state, not the original pre-move source state. So using the moved-to object for pullback is not generally equivalent to replaying the original move from the original source.
Therefore, i prefer the first method, i think it is enough, simple and good. Because move constructor itself should be something just move the data, not some heavy behavior, i think constructor_reverse_forw is useless most of time.
I'd opt for Method1 + opening a new issue to track this discussion and explain that we need a better fix.

Sure.

github-actions · 2026-03-31T13:31:07Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-03-31T13:51:42Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-04-02T17:17:31Z

clang-tidy review says "All clean, LGTM! 👍"

codecov · 2026-04-02T17:19:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

github-actions · 2026-04-04T13:07:50Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-04-04T15:36:11Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-04-05T16:42:47Z

clang-tidy review says "All clean, LGTM! 👍"

fogsong233 · 2026-04-08T08:45:51Z

@vgvassilev It seems that failed checks is not related to this pr. One is run the previously benchmark, which fails as expected, another may be the problem of envs

vgvassilev reviewed Mar 12, 2026

View reviewed changes

vgvassilev requested a review from guitargeek March 17, 2026 07:28

guitargeek requested changes Mar 17, 2026

View reviewed changes

vgvassilev force-pushed the fix-xvalue-reverse branch from 822b1f2 to 4713126 Compare March 21, 2026 16:00

vgvassilev force-pushed the fix-xvalue-reverse branch from 4713126 to 6caf896 Compare March 24, 2026 19:02

Fix implicit move returns in reverse mode

540a2e4

fogsong233 force-pushed the fix-xvalue-reverse branch from 6caf896 to 540a2e4 Compare March 31, 2026 13:23

Update reverse-mode tests for copy/move fallback

6e31554

Make implicit move return test Clang-version tolerant

2efdd74

Handle std::move in reverse-mode call args

e00a0b2

Drop stale valgrind XFAILs for move return tests

8e1eec0

Stabilize move-constructor return regression test

5971414

Conversation

fogsong233 commented Mar 12, 2026

Uh oh!

github-actions bot commented Mar 12, 2026

Uh oh!

vgvassilev Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

fogsong233 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

fogsong233 commented Mar 13, 2026

Uh oh!

vgvassilev commented Mar 17, 2026

Uh oh!

guitargeek commented Mar 17, 2026

Uh oh!

guitargeek commented Mar 17, 2026

Uh oh!

guitargeek left a comment

Choose a reason for hiding this comment

Uh oh!

fogsong233 commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fogsong233 commented Mar 17, 2026

Uh oh!

vgvassilev commented Mar 21, 2026

Uh oh!

github-actions bot commented Mar 21, 2026

Uh oh!

fogsong233 commented Mar 22, 2026

Uh oh!

vgvassilev commented Mar 23, 2026

Uh oh!

fogsong233 commented Mar 24, 2026

Uh oh!

vgvassilev commented Mar 24, 2026

Uh oh!

github-actions bot commented Mar 24, 2026

Uh oh!

fogsong233 commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgvassilev commented Mar 26, 2026

Uh oh!

fogsong233 commented Mar 26, 2026

Uh oh!

github-actions bot commented Mar 31, 2026

Uh oh!

github-actions bot commented Mar 31, 2026

Uh oh!

github-actions bot commented Apr 2, 2026

Uh oh!

codecov bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Apr 4, 2026

Uh oh!

github-actions bot commented Apr 4, 2026

Uh oh!

github-actions bot commented Apr 5, 2026

Uh oh!

fogsong233 commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fogsong233 commented Mar 17, 2026 •

edited

Loading

fogsong233 commented Mar 26, 2026 •

edited

Loading

codecov bot commented Apr 2, 2026 •

edited

Loading