Try to improve literal types #998

vaithak · 2024-07-22T15:22:00Z

This is related to the review comments of PR #975.

github-actions · 2024-07-22T15:28:33Z

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

vgvassilev · 2024-07-22T15:54:15Z

lib/Differentiator/BaseForwardModeVisitor.cpp

@@ -1008,8 +1008,11 @@ StmtDiff BaseForwardModeVisitor::VisitDeclRefExpr(const DeclRefExpr* DRE) {
  // If DRE is of type pointer, then the derivative is a null pointer.
  if (clonedDRE->getType()->isPointerType())
    return StmtDiff(clonedDRE, nullptr);
+  QualType literalTy = utils::GetValueType(clonedDRE->getType());


We should extend visitorbase::getZeroInit as discussed here #989 (comment)

cc: @gojakuch

vgvassilev · 2024-07-24T12:33:07Z

This pull request, if the review comment is addressed, should fix #1001.

kchristin22 · 2024-08-02T07:45:55Z

The cuda bug was a result of not handling the enum types correctly. clad::utils::GetValueType() doesn't return the underlying type of the enum but the enum itself (as a type). When checking isRealType() for that node, the cudaError_t enum passes the test so it's type is not set as an Int. This lead to a compilation error down the line.

I fixed that part in clad::utils::GetValueType() in a branch based on Vaibhav's cuda-bug branch. Should I open a new PR and reference this one and continue addressing the comments of this and its previous PR there?

vgvassilev · 2024-08-02T08:24:03Z

The cuda bug was a result of not handling the enum types correctly. clad::utils::GetValueType() doesn't return the underlying type of the enum but the enum itself (as a type). When checking isRealType() for that node, the cudaError_t enum passes the test so it's type is not set as an Int. This lead to a compilation error down the line.

I fixed that part in clad::utils::GetValueType() in a branch based on Vaibhav's cuda-bug branch. Should I open a new PR and reference this one and continue addressing the comments of this and its previous PR there?

Can you push to this PR?

github-actions · 2024-08-02T08:29:53Z

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

…a-bug

kchristin22 · 2024-08-03T09:27:50Z

The cuda bug was a result of not handling the enum types correctly. clad::utils::GetValueType() doesn't return the underlying type of the enum but the enum itself (as a type). When checking isRealType() for that node, the cudaError_t enum passes the test so it's type is not set as an Int. This lead to a compilation error down the line.
I fixed that part in clad::utils::GetValueType() in a branch based on Vaibhav's cuda-bug branch. Should I open a new PR and reference this one and continue addressing the comments of this and its previous PR there?

Can you push to this PR?

Sure, but I have no permissions in Vaibhav's repo, @vaithak could you change it temporarily(till this PR is merged)?

vgvassilev · 2024-08-03T12:54:09Z

If you accept my invite you should be able to push to this PR.

github-actions · 2024-08-03T13:40:58Z

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

codecov · 2024-08-03T13:42:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.40%. Comparing base (8f7d247) to head (0413ab7).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #998      +/-   ##
==========================================
- Coverage   94.41%   94.40%   -0.02%     
==========================================
  Files          55       55              
  Lines        8410     8430      +20     
==========================================
+ Hits         7940     7958      +18     
- Misses        470      472       +2

Files with missing lines	Coverage Δ
lib/Differentiator/BaseForwardModeVisitor.cpp	`98.53% <100.00%> (+<0.01%)`	⬆️
lib/Differentiator/CladUtils.cpp	`94.17% <100.00%> (-0.20%)`	⬇️
lib/Differentiator/ConstantFolder.cpp	`38.94% <100.00%> (+13.30%)`	⬆️
lib/Differentiator/VisitorBase.cpp	`96.90% <100.00%> (-0.01%)`	⬇️

... and 1 file with indirect coverage changes

Files with missing lines	Coverage Δ
lib/Differentiator/BaseForwardModeVisitor.cpp	`98.53% <100.00%> (+<0.01%)`	⬆️
lib/Differentiator/CladUtils.cpp	`94.17% <100.00%> (-0.20%)`	⬇️
lib/Differentiator/ConstantFolder.cpp	`38.94% <100.00%> (+13.30%)`	⬆️
lib/Differentiator/VisitorBase.cpp	`96.90% <100.00%> (-0.01%)`	⬇️

... and 1 file with indirect coverage changes

github-actions · 2024-08-07T14:41:04Z

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

github-actions

clang-tidy made some suggestions

github-actions · 2024-08-13T13:46:33Z

lib/Differentiator/VisitorBase.cpp

-      ExprResult Zero =
-          ConstantFolder::synthesizeLiteral(m_Context.IntTy, m_Context, 0);
+    else if (T->isScalarType() && !T->isPointerType()) {
+      ExprResult Zero = ConstantFolder::synthesizeLiteral(T, m_Context, 0);


warning: argument comment missing for literal argument 'val' [bugprone-argument-comment]

ExprResult Zero = ConstantFolder::synthesizeLiteral(T, m_Context, 0); ^

this fix will not be applied because it overlaps with another fix

lib/Differentiator/VisitorBase.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2024-08-13T14:14:52Z

lib/Differentiator/VisitorBase.cpp

-      ExprResult Zero =
-          ConstantFolder::synthesizeLiteral(m_Context.IntTy, m_Context, 0);
+    if (T->isScalarType() && !T->isPointerType()) {
+      ExprResult Zero = ConstantFolder::synthesizeLiteral(T, m_Context, 0);


warning: argument comment missing for literal argument 'val' [bugprone-argument-comment]

Suggested change

ExprResult Zero = ConstantFolder::synthesizeLiteral(T, m_Context, 0);

ExprResult Zero = ConstantFolder::synthesizeLiteral(T, m_Context, /*val=*/0);

…n necessary

github-actions

clang-tidy made some suggestions

github-actions · 2024-08-26T11:35:21Z

lib/Differentiator/ConstantFolder.cpp

@@ -128,14 +134,18 @@ namespace clad {
                                          uint64_t val) {
    //SourceLocation noLoc;
    Expr* Result = 0;
-    if (QT->isIntegralType(C)) {
+    if (QT->isBooleanType()) {
+      printf("synthesizing boolean literal\n");


warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg]

printf("synthesizing boolean literal\n"); ^

github-actions · 2024-08-26T11:35:21Z

lib/Differentiator/ConstantFolder.cpp

      llvm::APFloat APVal(C.getFloatTypeSemantics(QT), val);
      Result = clad::synthesizeLiteral(QT, C, APVal);
+    } else {
+      Result = ConstantFolder::synthesizeLiteral(C.IntTy, C, 0);


warning: argument comment missing for literal argument 'val' [bugprone-argument-comment]

Suggested change

Result = ConstantFolder::synthesizeLiteral(C.IntTy, C, 0);

Result = ConstantFolder::synthesizeLiteral(C.IntTy, C, /*val=*/0);

…sons

github-actions · 2024-08-26T14:21:34Z

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

This reverts commit 2e6b9d0.

github-actions · 2024-08-26T16:08:28Z

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

vgvassilev · 2024-08-26T18:22:44Z

lib/Differentiator/BaseForwardModeVisitor.cpp

@@ -1070,8 +1070,11 @@ StmtDiff BaseForwardModeVisitor::VisitDeclRefExpr(const DeclRefExpr* DRE) {
  // If DRE is of type pointer, then the derivative is a null pointer.
  if (clonedDRE->getType()->isPointerType())
    return StmtDiff(clonedDRE, nullptr);
+  QualType literalTy = utils::GetValueType(clonedDRE->getType());
+  if (!literalTy->isRealType())


Why do we need this special check here?

vgvassilev · 2024-08-26T18:23:49Z

lib/Differentiator/BaseForwardModeVisitor.cpp

+    if (!literalTy->isRealType())
+      literalTy = m_Context.IntTy;


I think we should not need this. This gets handled in ConstantFolder::synthesizeLiteral anyways.

True, it was a leftover from Vaibhav's PR. I will remove these lines.

vgvassilev · 2024-08-26T18:28:31Z

lib/Differentiator/VisitorBase.cpp

@@ -379,11 +379,10 @@ namespace clad {
    // FIXME: Consolidate other uses of synthesizeLiteral for creation 0 or 1.
    if (T->isVoidType())
      return nullptr;
-    if (T->isScalarType()) {
+    if (T->isScalarType() && !T->isPointerType()) {


Why do we need this here? Shouldn't ConstantFolder::synthesizeLiteral check if T is a pointer type and synthesize nullptr?

Right now, the pointers are initialized as an empty list, which is equivalent to nullptr, the same way clad tapes are initialized. I can alter them to be equal to nullptr if that's safer.

Yes, that'd be more readable, too.

lib/Differentiator/VisitorBase.cpp

… init

github-actions · 2024-09-02T21:27:12Z

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

github-actions · 2024-09-02T21:40:18Z

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

vgvassilev

We are almost there. See this one comment.

vgvassilev · 2024-09-03T06:20:56Z

lib/Differentiator/VisitorBase.cpp

-      CastKind CK = m_Sema.PrepareScalarCast(Zero, T);
-      return m_Sema.ImpCastExprToType(Zero.get(), T, CK).get();
+          ConstantFolder::synthesizeLiteral(T, m_Context, /*val=*/0);
+      return Zero.get();
    }
    return m_Sema.ActOnInitList(noLoc, {}, noLoc).get();


We should probably sink ActOnInitList into synthesizeLiteral but let's do it outside of this pr.

I think it makes sense to be in getZeroInit as we initialize with an empty list, while synthesizeLiteral is used in other places as well

vgvassilev · 2024-09-03T06:24:40Z

test/Gradient/FunctionCalls.C

-// CHECK-NEXT:         double _r0 = 0;
-// CHECK-NEXT:         char _r1 = 0;
+// CHECK-NEXT:         double _r0 = 0.;
+// CHECK-NEXT:         char _r1 = 0i8;


That will fail to compile I think.

Yes indeed, fixed

vgvassilev · 2024-09-03T06:31:07Z

test/Gradient/STLCustomDerivatives.C

 // CHECK-NEXT:         {{.*}}class_functions::operator_subscript_pullback(&_t26, 0, _r_d6, &_d_vec, &_r10);
-// CHECK-NEXT:         {{.*}} _r11 = 0;
+// CHECK-NEXT:         size_type _r11 = {{0U|0UL}};


That will not compile. We probably mean std::vector<...>::size_type. I do not think that's a problem for this PR.

I'll open an issue

github-actions · 2024-09-03T09:03:49Z

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

vgvassilev

LGTM!

Try to improve literal types

2b2af14

vgvassilev reviewed Jul 22, 2024

View reviewed changes

Try to improve literal types

61982a1

vgvassilev force-pushed the cuda-bug branch from 2b2af14 to 61982a1 Compare August 2, 2024 08:24

kchristin22 added 2 commits August 3, 2024 11:46

Merge remote-tracking branch 'refs/remotes/vaithak/cuda-bug' into cud…

4432e1f

…a-bug

Get enum's underlying value type

4b57f76

vaithak and others added 2 commits August 7, 2024 17:34

Try to improve literal types

1f4e2a3

Get enum's underlying value type

97acf5f

vgvassilev force-pushed the cuda-bug branch from 4b57f76 to 97acf5f Compare August 7, 2024 14:34

kchristin22 added 6 commits August 8, 2024 12:14

Zero initialization based on type

aee91cd

Fix pointer zero initialization

6130c91

Merge with master

98352a8

Merge remote-tracking branch 'vaithak/cuda-bug' into cuda-bug

1de6bbe

Fix format

17012c0

Fix compatibility of zero initialization of unsigned ints in tests

9feae1b

github-actions bot reviewed Aug 13, 2024

View reviewed changes

Fix clang-tidy suggestions

03fbcec

github-actions bot reviewed Aug 13, 2024

View reviewed changes

kchristin22 and others added 2 commits August 21, 2024 14:42

Remove unnecessary leftover casting for zero initilization

1a1e25b

Merge branch 'master' into cuda-bug

2dc1126

kchristin22 added 2 commits August 21, 2024 15:26

Fix STL test

ac042f9

Synthesize booleans separately from integers and cast type to int whe…

69075f1

…n necessary

github-actions bot reviewed Aug 26, 2024

View reviewed changes

kchristin22 added 2 commits August 26, 2024 16:34

Create the boolean literal with the new keyword for compatibility rea…

3a593d0

…sons

Remove fail case of type for zero initialization

d3740e4

kchristin22 marked this pull request as ready for review August 26, 2024 15:30

kchristin22 added 2 commits August 26, 2024 18:47

Add complex type test

2e6b9d0

Revert "Add complex type test"

2a8655e

This reverts commit 2e6b9d0.

vgvassilev reviewed Aug 26, 2024

View reviewed changes

kchristin22 added 3 commits September 2, 2024 23:50

Remove check for nonRealType before calling synthesizeLiteral

df2617d

Initialize pointers as nullptr and add a default int case for literal…

6816d9a

… init

Merge branch 'master' into cuda-bug

f7b3caa

Fix clang format

c74882d

vgvassilev reviewed Sep 3, 2024

View reviewed changes

kchristin22 added 2 commits September 3, 2024 11:50

Init chars as ints to remove suffix

fd45301

Use AnyCharacterType check instead of plain CharType

0413ab7

vgvassilev approved these changes Sep 3, 2024

View reviewed changes

vgvassilev merged commit 4810b6d into vgvassilev:master Sep 3, 2024
89 checks passed

	ExprResult Zero = ConstantFolder::synthesizeLiteral(T, m_Context, 0);
	ExprResult Zero = ConstantFolder::synthesizeLiteral(T, m_Context, /val=/0);

	Result = ConstantFolder::synthesizeLiteral(C.IntTy, C, 0);
	Result = ConstantFolder::synthesizeLiteral(C.IntTy, C, /val=/0);

Try to improve literal types #998

Try to improve literal types #998

Conversation

vaithak commented Jul 22, 2024 • edited Loading

github-actions bot commented Jul 22, 2024

Choose a reason for hiding this comment

vgvassilev commented Jul 24, 2024

kchristin22 commented Aug 2, 2024

vgvassilev commented Aug 2, 2024

github-actions bot commented Aug 2, 2024

kchristin22 commented Aug 3, 2024

vgvassilev commented Aug 3, 2024

github-actions bot commented Aug 3, 2024

codecov bot commented Aug 3, 2024 • edited Loading

Codecov Report

github-actions bot commented Aug 7, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 13, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 13, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Aug 26, 2024

Choose a reason for hiding this comment

github-actions bot Aug 26, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 26, 2024

github-actions bot commented Aug 26, 2024

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

github-actions bot commented Sep 2, 2024

github-actions bot commented Sep 2, 2024

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

vgvassilev Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 3, 2024

vgvassilev left a comment

Choose a reason for hiding this comment

vaithak commented Jul 22, 2024 •

edited

Loading

codecov bot commented Aug 3, 2024 •

edited

Loading

vgvassilev Sep 3, 2024 •

edited

Loading