Optimize stores, fix issues with compound statements in reverse mode #129
Conversation
| @@ -484,8 +489,33 @@ namespace clad { | |||
| clang::Expr* GlobalStoreAndRef(clang::Expr* E, | |||
| llvm::StringRef prefix = "_t"); | |||
|
|
|||
| //// A reference to the output parameter of the gradient function. | |||
| clang::Expr* m_Result; | |||
| struct DelayedStoreResult { | |||
There was a problem hiding this comment.
Could we default initialize the members of the struct?
There was a problem hiding this comment.
I do not think it is a good idea, having default member initializers before C++14 does not allow to use aggregate initialization, and makes us write constructors. This class is not intended to be default-initialized, it is allways constructed with all members provided.
| ReverseModeVisitor::forward); | ||
| } | ||
| }; | ||
| /// Sometimes (e.g. when visiting multiplication/division operator), we |
There was a problem hiding this comment.
Shouldn't that documentation be moved to the class definition?
| /// blocks. First result is a StmtDiff of forward/reverse blocks with | ||
| /// additionally created Stmts, second is a direct result of call to Visit. | ||
| std::pair<StmtDiff, StmtDiff> | ||
| DifferentiateSingleExpr(const clang::Expr* E, clang::Expr* expr = nullptr); |
There was a problem hiding this comment.
Could we use more meaningful param names? Waht is expr?
There was a problem hiding this comment.
It just a wrapper that forwards its arguments to Visit(E, expr), where expr is current accumulated derivative w.r.t. E. I don't think I have a better explanation.
There was a problem hiding this comment.
I meant to ask to change the parameter name to dfdx or smth like that.
| bool UsefulToStore(Expr* E) { | ||
|
|
||
| static bool UsefulToStore(Expr* E) { |
There was a problem hiding this comment.
Can we add some more documentation to this routine?
| CompoundStmt* RCS = endBlock(reverse); | ||
| Stmt* ForwardResult = endBlock(forward); | ||
| Stmt* ReverseResult = nullptr; | ||
| if (RCS->body_empty()) |
There was a problem hiding this comment.
Can't we use unwrapIfSingleStmt to simplify this block?
| Expr* ReverseModeVisitor::GlobalStoreAndRef(Expr* E, QualType Type, | ||
| llvm::StringRef prefix) { | ||
| static bool UsefulToStoreGlobal(Expr* E) { | ||
| // TODO: it may be worth to be more strict and use this, when storing in a |
There was a problem hiding this comment.
Could we use a single greppable form eg: TODO->FIXME.
efremale
force-pushed
the
optimize-store
branch
3 times, most recently
from
d3dd22a
to
91e70c3
Compare
Several optimizations for `StoreAndRef` and `GlobalStoreAndRef` are implemented. In `StoreAndRef`, useless temporaries are no longer created for expressions like `+x`, `-x` and array subscripts. In `GlobalStoreAndRef`, constant literals are no longer stored. This will be useful when implementing for-loops to avoid stores into stack. Additionally, in the reverse mode, an issue when visiting conditional operators is fixed: In C++, in conditional operators like `(c ? a : b)` only `a` or `b` is executed, depending on `c`. We did not account for this when visiting, therefore side effects of both `a` and `b` could be applied, independent of `c`. Now, an IfStmt is created and only one side will be evaluated.
Several optimizations for
StoreAndRefandGlobalStoreAndRefare implemented.In
StoreAndRef, useless temporaries are no longer created for expressions like+x,-xand array subscripts. This fixed issue #89.In
GlobalStoreAndRef, constant literals are no longer stored. This will be useful when implementing for-loops to avoid stores into stack.Additionally, in the reverse mode, an issue when visiting conditional operators is fixed:
In C++, in conditional operators like
(c ? a : b)onlyaorbis executed, depending onc. We did not account for this when visiting, thereforeside effects of both
aandbcould be applied, independent ofc. Now, an IfStmt is created and only one side will be evaluated. The issue is still present in forward mode: issue #128.