Initialize adjoints of aggregate types with init lists #1163

PetroZarytskyi · 2024-12-05T18:04:23Z

With aggregate types, we don't have to rely on the default initialization or custom constructor forward passes to zero-initialize its fields:

struct aggrStruct {
  double a = 1;
  float b;
  ...
};

This option is incorrect since _d_x.a will be initialized to 1:

aggrStruct _d_x{};

This option could be correct but is just unnecessary:

... _t1 = custom_reverse_forward(...);
aggrStruct _d_x = _t1.adjoint; // unnecessary

This PR instead implements a simpler solution:

aggrStruct _d_x{0., 0.F};

codecov · 2024-12-05T18:15:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.56%. Comparing base (eee6faa) to head (852b4dc).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1163      +/-   ##
==========================================
+ Coverage   94.55%   94.56%   +0.01%     
==========================================
  Files          51       51              
  Lines        8919     8947      +28     
==========================================
+ Hits         8433     8461      +28     
  Misses        486      486

Files with missing lines	Coverage Δ
lib/Differentiator/ReverseModeVisitor.cpp	`95.59% <100.00%> (+0.03%)`	⬆️
lib/Differentiator/VisitorBase.cpp	`97.67% <100.00%> (+0.02%)`	⬆️

Files with missing lines	Coverage Δ
lib/Differentiator/ReverseModeVisitor.cpp	`95.59% <100.00%> (+0.03%)`	⬆️
lib/Differentiator/VisitorBase.cpp	`97.67% <100.00%> (+0.02%)`	⬆️

lib/Differentiator/ReverseModeVisitor.cpp

lib/Differentiator/VisitorBase.cpp

vgvassilev · 2024-12-07T08:10:54Z

lib/Differentiator/ReverseModeVisitor.cpp

@@ -4286,6 +4280,10 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context,
    if (Expr* customReverseForwFnCall = BuildCallToCustomForwPassFn(
            CE->getConstructor(), primalArgs, reverseForwAdjointArgs,
            /*baseExpr=*/nullptr)) {
+      if (RD->isAggregate())
+        diag(DiagnosticsEngine::Warning, CE->getBeginLoc(),


Is here the place where we found the custom forward reverse function and we diagnose we do not need it? If not we should move the check there, and point to the declaration itself with a DiagnosticsEngine::Note(CE->getDecl()->getBeginLoc()). And of course we should add a test for it...

I kept the diagnostics in the same place but now the location comes from the declaration. Is it better now?

We need both a warning on the call site and a note on our he definition. We also need a test.

github-actions

clang-tidy made some suggestions

github-actions · 2024-12-09T07:44:59Z

lib/Differentiator/VisitorBase.cpp

@@ -418,6 +418,13 @@ namespace clad {
      Expr* zero = ConstantFolder::synthesizeLiteral(T, m_Context, /*val=*/0);
      return m_Sema.ActOnInitList(noLoc, {zero}, noLoc).get();
    }
+    if (const auto* RD = T->getAsCXXRecordDecl())
+      if (RD->hasDefinition() && !RD->isUnion() && RD->isAggregate()) {
+        llvm::SmallVector<Expr*, 4> adjParams;


warning: no header providing "llvm::SmallVector" is directly included [misc-include-cleaner]

lib/Differentiator/VisitorBase.cpp:28:

- #include <numeric> + #include <llvm/ADT/SmallVector.h> + #include <numeric>

github-actions · 2024-12-09T07:44:59Z

lib/Differentiator/VisitorBase.cpp

+    if (const auto* RD = T->getAsCXXRecordDecl())
+      if (RD->hasDefinition() && !RD->isUnion() && RD->isAggregate()) {
+        llvm::SmallVector<Expr*, 4> adjParams;
+        for (const FieldDecl* FD : RD->fields())


warning: no header providing "clang::FieldDecl" is directly included [misc-include-cleaner]

for (const FieldDecl* FD : RD->fields()) ^

github-actions · 2024-12-09T08:02:45Z

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

github-actions

clang-tidy made some suggestions

lib/Differentiator/ReverseModeVisitor.cpp

vgvassilev · 2024-12-10T07:23:13Z

test/Gradient/UserDefinedTypes.C

+}}}
+
+double fn15(double x, double y) {
+  std::array<double, 2> arr; // expected-warning {{'std::array<double, 2>' is an aggregate type and its constructor does not require a user-defined forward sweep function}}


This does not need to be a std::array because we will remove its reverse function. It can be something as simple as an empty class...

github-actions

clang-tidy made some suggestions

github-actions · 2024-12-10T19:27:22Z

lib/Differentiator/ReverseModeVisitor.cpp

@@ -34,7 +34,9 @@
 #include <clang/AST/OperationKinds.h>
 #include <clang/Sema/Ownership.h>

+#include "llvm/ADT/SmallString.h"
 #include "llvm/Support/SaveAndRestore.h"


warning: included header SmallString.h is not used directly [misc-include-cleaner]

Suggested change

#include "llvm/Support/SaveAndRestore.h"

#include "llvm/Support/SaveAndRestore.h"

vgvassilev

Lgtm!

PetroZarytskyi force-pushed the triv-constr branch from f29e480 to 8857806 Compare December 5, 2024 21:26

vgvassilev reviewed Dec 6, 2024

View reviewed changes

lib/Differentiator/ReverseModeVisitor.cpp Show resolved Hide resolved

vgvassilev reviewed Dec 6, 2024

View reviewed changes

lib/Differentiator/VisitorBase.cpp Show resolved Hide resolved

PetroZarytskyi force-pushed the triv-constr branch 2 times, most recently from c429677 to 17786f2 Compare December 6, 2024 22:40

vgvassilev reviewed Dec 7, 2024

View reviewed changes

PetroZarytskyi force-pushed the triv-constr branch from 17786f2 to c685d4d Compare December 7, 2024 20:17

Initialize adjoints of aggregate types with init lists

95300e1

PetroZarytskyi force-pushed the triv-constr branch from c685d4d to 95300e1 Compare December 9, 2024 07:38

github-actions bot reviewed Dec 9, 2024

View reviewed changes

PetroZarytskyi added 2 commits December 9, 2024 09:56

produce both a warning and a note

c417950

make clang-tidy happy with the headers

8b20c3a

github-actions bot reviewed Dec 9, 2024

View reviewed changes

lib/Differentiator/ReverseModeVisitor.cpp Show resolved Hide resolved

lib/Differentiator/ReverseModeVisitor.cpp Show resolved Hide resolved

vgvassilev reviewed Dec 9, 2024

View reviewed changes

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

vgvassilev reviewed Dec 10, 2024

View reviewed changes

PetroZarytskyi force-pushed the triv-constr branch from d96937c to e63086f Compare December 10, 2024 19:20

github-actions bot reviewed Dec 10, 2024

View reviewed changes

PetroZarytskyi force-pushed the triv-constr branch 3 times, most recently from 6797990 to 6e4e5a7 Compare December 10, 2024 20:51

warn fix

852b4dc

PetroZarytskyi force-pushed the triv-constr branch from 6e4e5a7 to 852b4dc Compare December 10, 2024 20:57

vgvassilev approved these changes Dec 10, 2024

View reviewed changes

vgvassilev merged commit 47b6504 into vgvassilev:master Dec 10, 2024
89 of 90 checks passed

PetroZarytskyi deleted the triv-constr branch December 13, 2024 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize adjoints of aggregate types with init lists #1163

Initialize adjoints of aggregate types with init lists #1163

PetroZarytskyi commented Dec 5, 2024

codecov bot commented Dec 5, 2024 •

edited

Loading

vgvassilev Dec 7, 2024

PetroZarytskyi Dec 7, 2024

vgvassilev Dec 7, 2024

github-actions bot left a comment

github-actions bot Dec 9, 2024

github-actions bot Dec 9, 2024

github-actions bot commented Dec 9, 2024

github-actions bot left a comment

vgvassilev Dec 10, 2024

github-actions bot left a comment

github-actions bot Dec 10, 2024

vgvassilev left a comment

	#include "llvm/Support/SaveAndRestore.h"
	#include "llvm/Support/SaveAndRestore.h"

Initialize adjoints of aggregate types with init lists #1163

Initialize adjoints of aggregate types with init lists #1163

Conversation

PetroZarytskyi commented Dec 5, 2024

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

vgvassilev Dec 7, 2024

Choose a reason for hiding this comment

PetroZarytskyi Dec 7, 2024

Choose a reason for hiding this comment

vgvassilev Dec 7, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Dec 9, 2024

Choose a reason for hiding this comment

github-actions bot Dec 9, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 9, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

vgvassilev Dec 10, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Dec 10, 2024

Choose a reason for hiding this comment

vgvassilev left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 5, 2024 •

edited

Loading