Store reused expressions in intermediate variables instead of reevaluating them in forward mode. Remove unnecessary cloning in forward mode. Refactor DerivativeBuilder.cpp #80

efremale · 2018-08-12T14:27:46Z

Forward mode now supports storing expression results in intermediate
variables. This allows to avoid reevaluation if derived expressions are
used on several places. For example, for:

double t = std::sin(x) * std::cos(x);

We now produce:

double _t0 = std::sin(x);
double _t1 = std::cos(x);
double _d_t = sin_darg0(x) * (_d_x) * _t1 + _t0 * cos_darg0(x) * (_d_x);
double t = _t0 * _t1;

Instead of:

double _d_t = sin_darg0(x) * (_d_x) * cos(x) + sin(x) * cos_darg0(x) * (_d_x);
double t = sin(x) * cos(x);

which allows us to avoid reevaluation of sin and cos.

This commit solves the issue #47 in case of forward mode.
For example, when deriving BinaryOperator, instead of cloning its whole
tree and visiting it (potentially cloning the subtrees again), e.g.

auto ClonedBO = Clone(BO); // both LHS and RHS are cloned
auto LHS = ClonedBO->getLHS();
auto LHSDiff = Visit(LHS) // LHS will be cloned inside again
auto RHS = ClonedBO->getRHS();
auto RHSDiff = Visit(RHS);
auto BODiff = ...// Build BODiff from LHSDiff and RHSDiff

we first visit the whole tree and rebuild the original operation from
the results of subtrees, e.g.

auto [LHS, LHSDiff] = Visit(BO->getLHS());
auto [RHS, RHSDiff] = Visit(BO->getRHS());
auto ClonedBO = BuildOp(OpKind, LHS, RHS);
auto BODiff = ...// Build BODiff from LHSDiff and RHSDiff.

Some refactoring was applied to DerivativeBuilder.cpp to support new way
of visiting. Some functions of DerivativeBuilder and Visitors were moved
to VisitorBase.

efremale · 2018-08-12T14:28:19Z

include/clad/Differentiator/DerivativeBuilder.h

-      return llvm::cast<T>(getStmt());
-    }
-  };
-


Replaced by StmtDiff class

efremale · 2018-08-12T14:28:47Z

include/clad/Differentiator/DerivativeBuilder.h

-    }
-  };
-
+  static clang::SourceLocation noLoc{};


Moved from DerivativeBuilder.cpp to DerivativeBuilder.h

efremale · 2018-08-12T14:29:06Z

include/clad/Differentiator/DerivativeBuilder.h

-      clang::BinaryOperatorKind OpCode,
-      clang::Expr* L,
-      clang::Expr* R);
-


Moved to VisitorBase class

efremale · 2018-08-12T14:30:20Z

include/clad/Differentiator/DerivativeBuilder.h

+    }
+    /// Output a statement to the current block.
+    void emitToCurrentBlock(clang::Stmt* S) {
+      currentBlock().push_back(S);
    }


Moved from ReverseModeVisitor to VisitorBase

efremale · 2018-08-12T14:31:03Z

include/clad/Differentiator/DerivativeBuilder.h

-    // Shorthands that delegate their functionality to DerviativeBuilder.
-    // Used to simplify the code.
-    clang::Stmt* Clone(const clang::Stmt* S);
-    clang::Expr* Clone(const clang::Expr* E);



Moved to VisitorBase

vgvassilev

Here are some comments from the partial review.

Overall I like the direction. This PR adds a few new features!

It seems that some of the tests are broken by the new functionality. We compute wrongly the derivatives of some functions. I suspect we are missing braces in few places to capture the right operations priorities.

I have added a few style comments inline. The gist is:

We should not overuse auto -- if the casual reader cannot deduce (easily) the type of the RHS of the expression we should not use auto.
We should not overuse brace-init -- this saves time but makes the code less readable because prevents us to 'inject' more meta information in the form of local variables.
We should indent only when it is necessary -- we should try to use the space on the same line as much as we can and break only if we need to.

vgvassilev · 2018-08-13T16:08:12Z

include/clad/Differentiator/DerivativeBuilder.h

+      data[0] = diff;
+    }
+
+    clang::Stmt* Stmt() { return data[1]; }


The name of the methods should contain a verb. This should be something like getOriginalStmt()...

Please update accordingly the rest of the names.

vgvassilev · 2018-08-15T07:26:06Z

include/clad/Differentiator/DerivativeBuilder.h

@@ -7,9 +7,11 @@
 #ifndef CLAD_DERIVATIVE_BUILDER_H
 #define CLAD_DERIVATIVE_BUILDER_H

+#include "clang/Sema/Sema.h"


Please order the includes alphabetically.

vgvassilev · 2018-08-15T07:28:15Z

include/clad/Differentiator/DerivativeBuilder.h

+    }
+
+    /// Get the latest block of code (i.e. place for statements output).
+    Stmts & currentBlock() {


Could you rename this to getCurrentBlock, getCurBlock (to be consistent in naming) or getActiveBlock (see comments below).

vgvassilev · 2018-08-15T07:29:12Z

include/clad/Differentiator/DerivativeBuilder.h

+      return m_Blocks.top();
+    }
+    /// Create new block.
+    Stmts & startBlock() {


I personally find beginBlock/endBlock conveying better the intent.

vgvassilev · 2018-08-15T07:30:56Z

include/clad/Differentiator/DerivativeBuilder.h

+      return CS;
+    }
+    /// Output a statement to the current block.
+    void emitToCurrentBlock(clang::Stmt* S) {


In clang lingo emit is often used in the context of codegen. Could we rename the method to addToCurrentBlock or addToActiveBlock?

vgvassilev · 2018-08-15T08:41:59Z

lib/Differentiator/DerivativeBuilder.cpp

-                                         elseBody);
-    currentBlock().push_back(ifStmt);  
+    auto ifStmt =
+      new (m_Context) IfStmt(m_Context,


Move to prev line.

vgvassilev · 2018-08-15T09:03:04Z

test/FirstDerivative/BasicArithmeticMulDiv.C

+// CHECK-NEXT: int _t0 = x * y;
+// CHECK-NEXT: int _t1 = _t0 * x;
+// CHECK-NEXT: int _t2 = _t1 * 3;
+// CHECK-NEXT: return 1 * y + x * _d_y * x + _t0 * 1 * 3 + _t1 * 0 * x + _t2 * 1;


This does not seem to produce 9x^2y.

vgvassilev · 2018-08-15T09:03:55Z

test/FirstDerivative/BasicArithmeticMulDiv.C

-// CHECK-NEXT: x *= 0.;
-// CHECK-NEXT: return 1.;
-// CHECK-NEXT: }
+// FIXME: +=, etc. operators are not currently supported, ignore for now.


Could you open a ticket for this?

vgvassilev · 2018-08-15T09:08:22Z

test/FirstDerivative/BasicArithmeticMulDiv.C

+// CHECK-NEXT: int _t2 = _t1 * y;
+// CHECK-NEXT: int _t3 = _t2 / y;
+// CHECK-NEXT: int _t4 = _t3 * 3;
+// CHECK-NEXT: return (((1 * x + x * 1 * x - _t0 * 1) / (x * x) * y + _t1 * _d_y * y - _t2 * _d_y) / (y * y) * 3 + _t3 * 0 * 3 - _t4 * 0) / (3 * 3);


Look like this does not evaluate to 1 but to 1/3xy see here.

vgvassilev · 2018-08-15T09:17:47Z

test/FirstDerivative/CallArguments.C

@@ -18,39 +18,40 @@ float g(float x) {
 }

 // CHECK: float g_darg0(float x) {
-// CHECK-NEXT: return f_darg0(x * x * x)  * (((1.F * x + x * 1.F) * x + x * x * 1.F));
+// CHECK: float _t0 = x * x;
+// CHECK-NEXT: f_darg0(_t0 * x) * (1.F * x + x * 1.F * x + _t0 * 1.F);


The second term of the expression seems to evaluate to 2x^2*x instead of 3x^2.

vgvassilev

Thank you for improving the code!

I have added a few comments. The gradient computations seem to deviate from the reference files. I have not investigated if the new values are correct and the old wrong or vice versa.

Please add your new changes in a separate commit. This will save me time because I have validated all changes in the test suite. This way I will only look at the changes you made and re-verify the relevant tests. Before merging we will squash all in one.

vgvassilev · 2018-08-19T12:59:14Z

include/clad/Differentiator/DerivativeBuilder.h

@@ -8,8 +8,10 @@
 #define CLAD_DERIVATIVE_BUILDER_H

 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Sema/Sema.h"


Move it after the StmtVisitor.h please.

vgvassilev · 2018-08-19T13:01:46Z

include/clad/Differentiator/DerivativeBuilder.h

-    clang::Expr* BuildOp(clang::UnaryOperatorKind OpCode, clang::Expr* E) {
-      return m_Builder.BuildOp(OpCode, E);
+    /// Get the latest block of code (i.e. place for statements output).
+    Stmts & getCurrentBlock() {


Could you put the & next to Stmts?

vgvassilev · 2018-08-19T13:05:37Z

include/clad/Differentiator/DerivativeBuilder.h

+    }
+    /// Remove the block from the stack, wrap it in CompoundStmt and return it.
+    clang::CompoundStmt* endBlock() {
+      auto CS = MakeCompoundStmt(getCurrentBlock());


Looks like we are suboptimal here in the case where we have only 1 statement in the block. Perhaps all current uses do not leave just a single statement in a block but let's protect it from our-future-selves.

In many cases, it is actually desirable to get a CompoundStmt, even if it has only 1 statement inside. E.g. when visiting another CompoundStmt (especially a function body).

vgvassilev · 2018-08-19T13:11:07Z

include/clad/Differentiator/DerivativeBuilder.h

+    clang::Stmt* BuildDeclStmt(llvm::MutableArrayRef<clang::Decl*> DS);
+
+    /// Builds a DeclRefExpr to a given Decl.
+    clang::Expr* BuildDeclRef(clang::VarDecl* D);


Looks like Sema::BuildDeclRefExpr returns a DeclRefExpr, could you narrow the return type of the routine?

Sema::BuildDeclRefExpr returns ExprResult (alias for ActionResult<Expr*>), and .get() on it returns Expr*. We can cast it to DeclRefExpr*, though it seems to be unnecessary since in most of the cases getting Expr* is sufficient.

This is a generic concept in Sema. Here we know what we expect. The return type narrowing has a lot of advantages from making the code clearer to detecting silent change of behavior.

Are we sure that BuildDeclRefExpr always returns DeclRefExpr though? For example, BuildBinOp may return either BinaryOperator or CXXOperatorCallExpr.

The latest commit narrows return types.

vgvassilev · 2018-08-19T13:12:49Z

include/clad/Differentiator/DerivativeBuilder.h

+                                 bool DirectInit = false);
+    /// Wraps a declaration in DeclStmt.
+    clang::Stmt* BuildDeclStmt(clang::Decl* D);
+    clang::Stmt* BuildDeclStmt(llvm::MutableArrayRef<clang::Decl*> DS);


Could you narrow the return type to clang::DeclStmt in both overloads?

Similarly to the above, with StmtResult/Stmt*.

vgvassilev · 2018-08-19T14:27:25Z

test/FirstDerivative/NonContinuous.C

 // CHECK-NEXT:   return 0;
-// CHECK-NEXT: else
+// CHECK-NEXT:  } else {


vgvassilev · 2018-08-19T14:27:37Z

test/FirstDerivative/Recursive.C

@@ -15,10 +15,11 @@ int f_pow(int arg, int p) {
 }

 // CHECK: int f_pow_darg0(int arg, int p) {
-// CHECK-NEXT:  if (p == 0)
+// CHECK-NEXT:  if (p == 0) {


vgvassilev · 2018-08-19T14:38:02Z

test/Misc/RunDemos.C

+// CHECK_GRADIENT: float _t3 = (y - yc);
+// CHECK_GRADIENT: float _t4 = (z - zc);
+// CHECK_GRADIENT: float _t5 = (z - zc);
+// CHECK_GRADIENT: return (1.F - 0.F) * _t1 + _t0 * (1.F - 0.F) + (0.F - 0.F) * _t3 + _t2 * (0.F - 0.F) + (0.F - 0.F) * _t5 + _t4 * (0.F - 0.F) - (0.F * r + r * 0.F);


IIUC, this yields different result.

It seems to be the same.

Indeed, I screwed up checking.

vgvassilev · 2018-08-19T14:42:03Z

test/Misc/RunDemos.C

+// CHECK_GRADIENT: float _t2 = (y - yc);
+// CHECK_GRADIENT: float _t3 = (y - yc);
+// CHECK_GRADIENT: float _t4 = (z - zc);
+// CHECK_GRADIENT: float _t5 = (z - zc);


Looks like _t5, _t3 and _t1 are redundant.

Since the original function uses (x - xc) on several places, we get several temporaries for it. We do not check the original function for repeated expressions, it would add quite a huge computational overhead. Moreover, since it was acceptable for the original function to use the same expression repeatedly, it is presumably acceptable to do so in the derivative.

I.e. the original function is

return (x-xc)*(x-xc) + (y-yc)*(y-yc) + (z-zc)*(z-zc) - r*r;

instead of

auto t0 = (x - xc); auto t1 = (y - yc); auto t2 = (z - zc); return t0 * t0 + t1 * t1 + t2 * t2 - r * r;

Avoiding redundant temporaries would require us to transform former to later, which is computationally expensive in general, we should hope that the one who writes the original function takes care of this.

Ok, let's resolve this by opening an issue to document this.

vgvassilev · 2018-08-19T14:44:41Z

test/Misc/RunDemos.C

+// CHECK_GRADIENT: float _t3 = (y - yc);
+// CHECK_GRADIENT: float _t4 = (z - zc);
+// CHECK_GRADIENT: float _t5 = (z - zc);
+// CHECK_GRADIENT: return (0.F - 0.F) * _t1 + _t0 * (0.F - 0.F) + (0.F - 0.F) * _t3 + _t2 * (0.F - 0.F) + (1.F - 0.F) * _t5 + _t4 * (1.F - 0.F) - (0.F * r + r * 0.F);


It seems to be the same.

vgvassilev · 2018-08-19T19:18:39Z

include/clad/Differentiator/DerivativeBuilder.h

@@ -169,7 +169,12 @@ namespace clad {
    template <std::size_t N>
    void diag(clang::DiagnosticsEngine::Level level, // Warning or Error
              const char (&format)[N],
-              llvm::ArrayRef<llvm::StringRef> args = {},
+              llvm::ArrayRef<llvm::StringRef> args,
+              clang::SourceLocation loc = {});


We must not provide easy-to-use API for emission of diagnostics with no source location. This makes them quite useless.

Forward mode now supports storing expression results in intermediate variables. This allows to avoid reevaluation if derived expressions are used on several places. For example, for: ``` double t = std::sin(x) * std::cos(x); ``` We now produce: ``` double _t0 = std::sin(x); double _t1 = std::cos(x); double _d_t = sin_darg0(x) * (_d_x) * _t1 + _t0 * cos_darg0(x) * (_d_x); double t = _t0 * _t1; ``` Instead of: ``` double _d_t = sin_darg0(x) * (_d_x) * cos(x) + sin(x) * cos_darg0(x) * (_d_x); double t = sin(x) * cos(x); ``` which allows us to avoid reevaluation of sin and cos. This commit also removes unnecessary cloning in forward mode, solving the issue vgvassilev#47 in case of forward mode. For example, when deriving BinaryOperator, instead of cloning its whole tree and visiting it (potentially cloning the subtrees again), e.g. ``` auto ClonedBO = Clone(BO); // both LHS and RHS are cloned auto LHS = ClonedBO->getLHS(); auto LHSDiff = Visit(LHS) // LHS will be cloned inside again auto RHS = ClonedBO->getRHS(); auto RHSDiff = Visit(RHS); auto BODiff = ...// Build BODiff from LHSDiff and RHSDiff ``` we first visit the whole tree and rebuild the original operation from the results of subtrees, e.g. ``` auto [LHS, LHSDiff] = Visit(BO->getLHS()); auto [RHS, RHSDiff] = Visit(BO->getRHS()); auto ClonedBO = BuildOp(OpKind, LHS, RHS); auto BODiff = ...// Build BODiff from LHSDiff and RHSDiff. ``` Some refactoring was applied to DerivativeBuilder.cpp to support new way of visiting. Some functions of DerivativeBuilder and Visitors were moved to VisitorBase.

efremale force-pushed the forward-mode-store-intermediate branch from 812d32e to e2dc98e Compare August 12, 2018 17:06

efremale commented Aug 12, 2018

View reviewed changes

efremale force-pushed the forward-mode-store-intermediate branch 5 times, most recently from 9f32086 to 71ace05 Compare August 12, 2018 21:27

vgvassilev requested changes Aug 15, 2018

View reviewed changes

efremale force-pushed the forward-mode-store-intermediate branch 3 times, most recently from bb23b07 to ddc36fd Compare August 18, 2018 22:15

vgvassilev requested changes Aug 19, 2018

View reviewed changes

efremale force-pushed the forward-mode-store-intermediate branch 4 times, most recently from d7f60c1 to 818d743 Compare August 19, 2018 18:22

vgvassilev reviewed Aug 19, 2018

View reviewed changes

efremale force-pushed the forward-mode-store-intermediate branch 3 times, most recently from d4be164 to f7dd27a Compare August 20, 2018 18:58

efremale added 4 commits August 20, 2018 21:52

Minor fixes

82e1217

Fix API for emission of diagnostics with no source location

f7dd27a

Narrow return types of Build... routines

567039c

vgvassilev approved these changes Aug 21, 2018

View reviewed changes

vgvassilev merged commit 3a058aa into vgvassilev:master Aug 21, 2018

Store reused expressions in intermediate variables instead of reevaluating them in forward mode. Remove unnecessary cloning in forward mode. Refactor DerivativeBuilder.cpp #80

Store reused expressions in intermediate variables instead of reevaluating them in forward mode. Remove unnecessary cloning in forward mode. Refactor DerivativeBuilder.cpp #80

Conversation

efremale commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgvassilev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgvassilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efremale Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgvassilev left a comment •

edited

Loading

efremale Aug 19, 2018 •

edited

Loading