Rev ref returns #601

PhrygianGates · 2023-07-20T17:56:02Z

This PR rebase the #425 pull-request on top of master.

codecov · 2023-07-28T23:26:42Z

Codecov Report

Merging #601 (ea8b406) into master (24a15f9) will increase coverage by 0.20%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #601      +/-   ##
==========================================
+ Coverage   93.83%   94.03%   +0.20%     
==========================================
  Files          41       43       +2     
  Lines        6032     6218     +186     
==========================================
+ Hits         5660     5847     +187     
+ Misses        372      371       -1

Files Changed	Coverage Δ
include/clad/Differentiator/Compatibility.h	`90.76% <ø> (ø)`
include/clad/Differentiator/DerivativeBuilder.h	`100.00% <ø> (ø)`
include/clad/Differentiator/ReverseModeVisitor.h	`98.70% <ø> (ø)`
...e/clad/Differentiator/ReverseModeForwPassVisitor.h	`100.00% <100.00%> (ø)`
lib/Differentiator/BaseForwardModeVisitor.cpp	`98.71% <100.00%> (-0.01%)`	⬇️
lib/Differentiator/DerivativeBuilder.cpp	`100.00% <100.00%> (ø)`
lib/Differentiator/ForwardModeVisitor.cpp	`99.06% <100.00%> (ø)`
lib/Differentiator/HessianModeVisitor.cpp	`99.46% <100.00%> (ø)`
lib/Differentiator/ReverseModeForwPassVisitor.cpp	`100.00% <100.00%> (ø)`
lib/Differentiator/ReverseModeVisitor.cpp	`95.73% <100.00%> (+0.13%)`	⬆️
... and 1 more

Files Changed	Coverage Δ
include/clad/Differentiator/Compatibility.h	`90.76% <ø> (ø)`
include/clad/Differentiator/DerivativeBuilder.h	`100.00% <ø> (ø)`
include/clad/Differentiator/ReverseModeVisitor.h	`98.70% <ø> (ø)`
...e/clad/Differentiator/ReverseModeForwPassVisitor.h	`100.00% <100.00%> (ø)`
lib/Differentiator/BaseForwardModeVisitor.cpp	`98.71% <100.00%> (-0.01%)`	⬇️
lib/Differentiator/DerivativeBuilder.cpp	`100.00% <100.00%> (ø)`
lib/Differentiator/ForwardModeVisitor.cpp	`99.06% <100.00%> (ø)`
lib/Differentiator/HessianModeVisitor.cpp	`99.46% <100.00%> (ø)`
lib/Differentiator/ReverseModeForwPassVisitor.cpp	`100.00% <100.00%> (ø)`
lib/Differentiator/ReverseModeVisitor.cpp	`95.73% <100.00%> (+0.13%)`	⬆️
... and 1 more

parth-07 · 2023-07-30T11:42:36Z

@PhrygianGates Can you please debug and fix the failing debug checks?

parth-07 · 2023-07-30T11:43:55Z

Is the pull request ready for review?

PhrygianGates · 2023-08-01T19:56:35Z

@PhrygianGates Can you please debug and fix the failing debug checks?

I'm not sure as it seems to go wrong when installing something. I'll push again to see what happens.

vgvassilev · 2023-08-01T20:00:55Z

@PhrygianGates Can you please debug and fix the failing debug checks?

I'm not sure as it seems to go wrong when installing something. I'll push again to see what happens.

This fails elsewhere due to some server outage it seems. We can ignore this for now.

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 40. Check the log or trigger a new build to see more.

include/clad/Differentiator/ReverseModeForwPassVisitor.h

github-actions · 2023-08-01T20:37:50Z

include/clad/Differentiator/ReverseModeForwPassVisitor.h

+#include "llvm/ADT/SmallVector.h"
+
+namespace clad {
+class ReverseModeForwPassVisitor : public ReverseModeVisitor {


warning: destructor of 'ReverseModeForwPassVisitor' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]

class ReverseModeForwPassVisitor : public ReverseModeVisitor { ^

Additional context

include/clad/Differentiator/ReverseModeForwPassVisitor.h:12: make it public and virtual

class ReverseModeForwPassVisitor : public ReverseModeVisitor { ^

github-actions · 2023-08-01T20:37:50Z

include/clad/Differentiator/ReverseModeForwPassVisitor.h

+private:
+  Stmts m_Globals;
+
+  llvm::SmallVector<clang::QualType, 8>


warning: 8 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]

llvm::SmallVector<clang::QualType, 8> ^

We should be able to suppress these by adding IgnorePowersOf2IntegerValues=true in the clang-tidy config: https://clang.llvm.org/extra/clang-tidy/checks/readability/magic-numbers.html

github-actions · 2023-08-01T20:37:50Z

include/clad/Differentiator/ReverseModeForwPassVisitor.h

+  llvm::SmallVector<clang::QualType, 8>
+  ComputeParamTypes(const DiffParams& diffParams);
+  clang::QualType ComputeReturnType();
+  llvm::SmallVector<clang::ParmVarDecl*, 8> BuildParams(DiffParams& diffParams);


warning: 8 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]

llvm::SmallVector<clang::ParmVarDecl*, 8> BuildParams(DiffParams& diffParams); ^

github-actions · 2023-08-01T20:37:50Z

lib/Differentiator/DerivativeBuilder.cpp

@@ -22,6 +22,9 @@
 #include "clad/Differentiator/HessianModeVisitor.h"
 #include "clad/Differentiator/JacobianModeVisitor.h"
 #include "clad/Differentiator/ReverseModeVisitor.h"
+#include "clad/Differentiator/ReverseModeForwPassVisitor.h"
+
+#include "clad/Differentiator/DiffPlanner.h"


warning: duplicate include [readability-duplicate-include]

lib/Differentiator/DerivativeBuilder.cpp:25:

- - #include "clad/Differentiator/DiffPlanner.h"

lib/Differentiator/ReverseModeForwPassVisitor.cpp

github-actions · 2023-08-01T20:37:51Z

lib/Differentiator/ReverseModeForwPassVisitor.cpp

+
+  llvm::SaveAndRestore<DeclContext*> saveContext(m_Sema.CurContext);
+  llvm::SaveAndRestore<Scope*> saveScope(m_CurScope);
+  m_Sema.CurContext = const_cast<DeclContext*>(m_Function->getDeclContext());


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

m_Sema.CurContext = const_cast<DeclContext*>(m_Function->getDeclContext()); ^

github-actions · 2023-08-01T20:37:51Z

lib/Differentiator/ReverseModeForwPassVisitor.cpp

+  m_Sema.CurContext = const_cast<DeclContext*>(m_Function->getDeclContext());
+
+  DeclWithContext fnBuildRes =
+      m_Builder.cloneFunction(m_Function, *this, m_Sema.CurContext, m_Sema,


warning: slicing object from type 'ReverseModeForwPassVisitor' to 'VisitorBase' discards 1096 bytes of state [cppcoreguidelines-slicing]

m_Builder.cloneFunction(m_Function, *this, m_Sema.CurContext, m_Sema, ^

github-actions · 2023-08-01T20:37:51Z

lib/Differentiator/ReverseModeForwPassVisitor.cpp

+  m_Sema.CurContext = const_cast<DeclContext*>(m_Function->getDeclContext());
+
+  DeclWithContext fnBuildRes =
+      m_Builder.cloneFunction(m_Function, *this, m_Sema.CurContext, m_Sema,


warning: slicing object from type 'ReverseModeForwPassVisitor' to 'VisitorBase' discards override 'VisitCompoundStmt' [cppcoreguidelines-slicing]

m_Builder.cloneFunction(m_Function, *this, m_Sema.CurContext, m_Sema, ^

github-actions · 2023-08-01T20:37:51Z

lib/Differentiator/ReverseModeForwPassVisitor.cpp

+  m_Sema.CurContext = const_cast<DeclContext*>(m_Function->getDeclContext());
+
+  DeclWithContext fnBuildRes =
+      m_Builder.cloneFunction(m_Function, *this, m_Sema.CurContext, m_Sema,


warning: slicing object from type 'ReverseModeForwPassVisitor' to 'VisitorBase' discards override 'VisitDeclRefExpr' [cppcoreguidelines-slicing]

m_Builder.cloneFunction(m_Function, *this, m_Sema.CurContext, m_Sema, ^

lib/Differentiator/ReverseModeForwPassVisitor.cpp

lib/Differentiator/DerivativeBuilder.cpp

lib/Differentiator/ReverseModeForwPassVisitor.cpp

lib/Differentiator/ReverseModeVisitor.cpp

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 31. Check the log or trigger a new build to see more.

lib/Differentiator/DerivativeBuilder.cpp

github-actions · 2023-08-09T11:28:06Z

lib/Differentiator/ReverseModeForwPassVisitor.cpp

+  m_Sema.CurContext = const_cast<DeclContext*>(m_Function->getDeclContext());
+
+  DeclWithContext fnBuildRes =
+      m_Builder.cloneFunction(m_Function, *this, m_Sema.CurContext, m_Sema,


warning: slicing object from type 'ReverseModeForwPassVisitor' to 'VisitorBase' discards override 'VisitReturnStmt' [cppcoreguidelines-slicing]

m_Builder.cloneFunction(m_Function, *this, m_Sema.CurContext, m_Sema, ^

github-actions · 2023-08-09T11:28:07Z

lib/Differentiator/ReverseModeForwPassVisitor.cpp

+  m_Sema.CurContext = const_cast<DeclContext*>(m_Function->getDeclContext());
+
+  DeclWithContext fnBuildRes =
+      m_Builder.cloneFunction(m_Function, *this, m_Sema.CurContext, m_Sema,


warning: slicing object from type 'ReverseModeForwPassVisitor' to 'VisitorBase' discards override 'VisitStmt' [cppcoreguidelines-slicing]

m_Builder.cloneFunction(m_Function, *this, m_Sema.CurContext, m_Sema, ^

lib/Differentiator/ReverseModeForwPassVisitor.cpp

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 21. Check the log or trigger a new build to see more.

lib/Differentiator/ReverseModeForwPassVisitor.cpp

github-actions · 2023-08-11T16:57:04Z

lib/Differentiator/ReverseModeForwPassVisitor.cpp

+  return T;
+}
+
+llvm::SmallVector<clang::ParmVarDecl*, 8>


warning: 8 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]

llvm::SmallVector<clang::ParmVarDecl*, 8> ^

lib/Differentiator/ReverseModeForwPassVisitor.cpp

github-actions · 2023-08-11T16:57:04Z

lib/Differentiator/ReverseModeForwPassVisitor.cpp

+
+llvm::SmallVector<clang::ParmVarDecl*, 8>
+ReverseModeForwPassVisitor::BuildParams(DiffParams& diffParams) {
+  llvm::SmallVector<clang::ParmVarDecl*, 8> params, paramDerivatives;


warning: 8 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]

llvm::SmallVector<clang::ParmVarDecl*, 8> params, paramDerivatives; ^

lib/Differentiator/ReverseModeForwPassVisitor.cpp

include/clad/Differentiator/ReverseModeForwPassVisitor.h

lib/Differentiator/ReverseModeVisitor.cpp

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 11. Check the log or trigger a new build to see more.

lib/Differentiator/ReverseModeForwPassVisitor.cpp

lib/Differentiator/ReverseModeVisitor.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2023-08-16T11:57:23Z

lib/Differentiator/ReverseModeForwPassVisitor.cpp

+
+llvm::SmallVector<clang::ParmVarDecl*, 8>
+ReverseModeForwPassVisitor::BuildParams(DiffParams& diffParams) {
+  llvm::SmallVector<clang::ParmVarDecl*, 8> params;


warning: 8 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]

llvm::SmallVector<clang::ParmVarDecl*, 8> params; ^

github-actions · 2023-08-16T11:57:23Z

lib/Differentiator/ReverseModeForwPassVisitor.cpp

+llvm::SmallVector<clang::ParmVarDecl*, 8>
+ReverseModeForwPassVisitor::BuildParams(DiffParams& diffParams) {
+  llvm::SmallVector<clang::ParmVarDecl*, 8> params;
+  llvm::SmallVector<clang::ParmVarDecl*, 8> paramDerivatives;


warning: 8 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]

llvm::SmallVector<clang::ParmVarDecl*, 8> paramDerivatives; ^

lib/Differentiator/ReverseModeVisitor.cpp

vgvassilev

Most of the clang-tidy suggestions make sense. Could you incorporate them in the PR?

include/clad/Differentiator/ReverseModeForwPassVisitor.h

vgvassilev · 2023-08-16T15:39:20Z

include/clad/Differentiator/ReverseModeForwPassVisitor.h

+#include "llvm/ADT/SmallVector.h"
+
+namespace clad {
+class ReverseModeForwPassVisitor : public ReverseModeVisitor {


demos/ComputerGraphics/smallpt/SmallPT.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2023-08-19T07:56:00Z

include/clad/Differentiator/Compatibility.h

+#define CLAD_COMPAT_CLANG11_LangOptions_EtraParams Ctx.getLangOpts()
+#endif
+#define CLAD_COMPAT_CLANG11_Ctx_ExtraParams Ctx,
+#define CLAD_COMPAT_CREATE11(CLASS, CTORARGS) (CLASS::Create CTORARGS)


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

#define CLAD_COMPAT_CREATE11(CLASS, CTORARGS) (CLASS::Create CTORARGS) ^

github-actions · 2023-08-19T07:56:00Z

tools/DerivedFnInfo.h

+  DiffInputVarsInfo m_DiffVarsInfo;
+  bool m_UsesEnzyme = false;
+
+  DerivedFnInfo() {}


warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]

Suggested change

DerivedFnInfo() {}

DerivedFnInfo() = default;

lib/Differentiator/ReverseModeForwPassVisitor.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2023-08-19T09:48:21Z

lib/Differentiator/DerivativeBuilder.cpp

-                                   clang::SourceLocation& noLoc,
-                                   clang::DeclarationNameInfo name,
-                                   clang::QualType functionType) {
+  DeclWithContext DerivativeBuilder::cloneFunction(


warning: method 'cloneFunction' can be made static [readability-convert-member-functions-to-static]

include/clad/Differentiator/DerivativeBuilder.h:94:

- DeclWithContext cloneFunction(const clang::FunctionDecl* FD, + static DeclWithContext cloneFunction(const clang::FunctionDecl* FD,

github-actions

clang-tidy made some suggestions

github-actions · 2023-08-22T12:07:27Z

include/clad/Differentiator/DerivativeBuilder.h

@@ -93,7 +93,7 @@ namespace clad {
    llvm::SmallVector<std::unique_ptr<ErrorEstimationHandler>, 4>
        m_ErrorEstHandler;
    DeclWithContext cloneFunction(const clang::FunctionDecl* FD,


warning: function 'clad::DerivativeBuilder::cloneFunction' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

DeclWithContext cloneFunction(const clang::FunctionDecl* FD, ^

Additional context

lib/Differentiator/DerivativeBuilder.cpp:93: the definition seen here

DeclWithContext DerivativeBuilder::cloneFunction( ^

include/clad/Differentiator/DerivativeBuilder.h:94: differing parameters are named here: ('VB'), in definition: ('VD')

DeclWithContext cloneFunction(const clang::FunctionDecl* FD, ^

lib/Differentiator/DerivativeBuilder.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2023-08-22T14:10:14Z

include/clad/Differentiator/Compatibility.h

@@ -427,7 +427,8 @@ static inline QualType getConstantArrayType(const ASTContext &Ctx,
 #if CLANG_VERSION_MAJOR < 10
   #define CLAD_COMPAT_CLANG10_FunctionDecl_Create_ExtraParams(x) /**/
 #elif CLANG_VERSION_MAJOR >= 10
-   #define CLAD_COMPAT_CLANG10_FunctionDecl_Create_ExtraParams(x) ,((x)?VD.Clone((x)):nullptr)
+#define CLAD_COMPAT_CLANG10_FunctionDecl_Create_ExtraParams(x)                 \


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

#define CLAD_COMPAT_CLANG10_FunctionDecl_Create_ExtraParams(x) \ ^

lib/Differentiator/ReverseModeForwPassVisitor.cpp

github-actions

clang-tidy made some suggestions

lib/Differentiator/ReverseModeForwPassVisitor.cpp

Co-authored-by: Daemond <daemondzh@gmail.com>

vgvassilev

LGTM. Thank you!

github-actions bot reviewed Aug 1, 2023

View reviewed changes

parth-07 requested changes Aug 4, 2023

View reviewed changes

github-actions bot reviewed Aug 9, 2023

View reviewed changes

PhrygianGates marked this pull request as ready for review August 11, 2023 13:17

github-actions bot reviewed Aug 11, 2023

View reviewed changes

parth-07 requested changes Aug 13, 2023

View reviewed changes

include/clad/Differentiator/ReverseModeForwPassVisitor.h Outdated Show resolved Hide resolved

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved

lib/Differentiator/ReverseModeVisitor.cpp Outdated Show resolved Hide resolved

github-actions bot reviewed Aug 14, 2023

View reviewed changes

github-actions bot reviewed Aug 16, 2023

View reviewed changes

PhrygianGates force-pushed the rev-ref-returns branch 2 times, most recently from e2b5e5f to d087419 Compare August 16, 2023 12:27

vgvassilev reviewed Aug 16, 2023

View reviewed changes

PhrygianGates force-pushed the rev-ref-returns branch from b6b52be to d89426c Compare August 19, 2023 07:12

parth-07 reviewed Aug 19, 2023

View reviewed changes

demos/ComputerGraphics/smallpt/SmallPT.cpp Outdated Show resolved Hide resolved

github-actions bot reviewed Aug 19, 2023

View reviewed changes

PhrygianGates force-pushed the rev-ref-returns branch from 1eebf49 to fab409d Compare August 19, 2023 08:22

vgvassilev force-pushed the rev-ref-returns branch from fab409d to 771d709 Compare August 19, 2023 08:27

PhrygianGates force-pushed the rev-ref-returns branch from 3d5503b to cc04f13 Compare August 19, 2023 08:42

vgvassilev force-pushed the rev-ref-returns branch 6 times, most recently from 0947893 to f3a7d33 Compare August 19, 2023 08:58

vgvassilev reviewed Aug 19, 2023

View reviewed changes

lib/Differentiator/ReverseModeForwPassVisitor.cpp Show resolved Hide resolved

vgvassilev force-pushed the rev-ref-returns branch from 321bd32 to 49d9aca Compare August 19, 2023 09:34

github-actions bot reviewed Aug 19, 2023

View reviewed changes

github-actions bot reviewed Aug 22, 2023

View reviewed changes

PhrygianGates force-pushed the rev-ref-returns branch from 17606c8 to 34a898d Compare August 22, 2023 12:35

vgvassilev force-pushed the rev-ref-returns branch 2 times, most recently from 89e33f9 to 0c0290e Compare August 22, 2023 13:42

github-actions bot reviewed Aug 22, 2023

View reviewed changes

vgvassilev force-pushed the rev-ref-returns branch from 0c0290e to 2c64861 Compare August 22, 2023 16:22

PhrygianGates force-pushed the rev-ref-returns branch from 2c64861 to cd842da Compare August 28, 2023 11:58

github-actions bot reviewed Aug 28, 2023

View reviewed changes

lib/Differentiator/ReverseModeForwPassVisitor.cpp Outdated Show resolved Hide resolved

PhrygianGates force-pushed the rev-ref-returns branch 2 times, most recently from 90e383d to a90bfc6 Compare August 28, 2023 13:46

Add support for diff of ref return types in rev mode

ea8b406

Co-authored-by: Daemond <daemondzh@gmail.com>

PhrygianGates force-pushed the rev-ref-returns branch from a90bfc6 to ea8b406 Compare August 28, 2023 13:47

vgvassilev approved these changes Aug 28, 2023

View reviewed changes

vgvassilev merged commit bba8cb1 into vgvassilev:master Aug 28, 2023

parth-07 mentioned this pull request Nov 12, 2023

Add initial support for diff of ref return types in rev mode #425

Closed

Rev ref returns #601

Rev ref returns #601

Conversation

PhrygianGates commented Jul 20, 2023 • edited Loading

codecov bot commented Jul 28, 2023 • edited Loading

Codecov Report

parth-07 commented Jul 30, 2023

parth-07 commented Jul 30, 2023

PhrygianGates commented Aug 1, 2023

vgvassilev commented Aug 1, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 1, 2023

Choose a reason for hiding this comment

vgvassilev Aug 16, 2023

Choose a reason for hiding this comment

github-actions bot Aug 1, 2023

Choose a reason for hiding this comment

vgvassilev Aug 2, 2023

Choose a reason for hiding this comment

github-actions bot Aug 1, 2023

Choose a reason for hiding this comment

github-actions bot Aug 1, 2023

Choose a reason for hiding this comment

github-actions bot Aug 1, 2023

Choose a reason for hiding this comment

github-actions bot Aug 1, 2023

Choose a reason for hiding this comment

github-actions bot Aug 1, 2023

Choose a reason for hiding this comment

github-actions bot Aug 1, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 9, 2023

Choose a reason for hiding this comment

github-actions bot Aug 9, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 11, 2023

Choose a reason for hiding this comment

github-actions bot Aug 11, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 16, 2023

Choose a reason for hiding this comment

github-actions bot Aug 16, 2023

Choose a reason for hiding this comment

vgvassilev left a comment

Choose a reason for hiding this comment

vgvassilev Aug 16, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 19, 2023

Choose a reason for hiding this comment

github-actions bot Aug 19, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 19, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 22, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 22, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

vgvassilev left a comment

Choose a reason for hiding this comment

PhrygianGates commented Jul 20, 2023 •

edited

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading