Fix the generation of invalid code in some common cases #1088

gojakuch · 2024-09-10T19:12:06Z

This commit fixes the way Clad generates code. Specifically, it addresses the way operators appear in the generated code in the reverse mode and the way nested name qualifiers are built in both modes. This includes a partial fix to #1050.

Fixes: #1087

github-actions

clang-tidy made some suggestions

lib/Differentiator/ReverseModeVisitor.cpp

lib/Differentiator/VisitorBase.cpp

github-actions · 2024-09-10T19:22:44Z

lib/Differentiator/VisitorBase.cpp

+        CSS.Extend(m_Context, NS, noLoc, noLoc);
+      } else if (NNS->getKind() == NestedNameSpecifier::TypeSpec) {
+        const Type* T = NNS->getAsType();
+        if (auto* RT = const_cast<RecordType*>(T->getAs<RecordType>())) {


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

if (auto* RT = const_cast<RecordType*>(T->getAs<RecordType>())) { ^

codecov · 2024-09-11T11:34:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.23%. Comparing base (8ed2707) to head (8243b3c).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
+ Coverage   94.21%   94.23%   +0.01%     
==========================================
  Files          48       48              
  Lines        8131     8158      +27     
==========================================
+ Hits         7661     7688      +27     
  Misses        470      470

Files with missing lines	Coverage Δ
include/clad/Differentiator/VisitorBase.h	`100.00% <ø> (ø)`
lib/Differentiator/BaseForwardModeVisitor.cpp	`98.73% <100.00%> (+<0.01%)`	⬆️
lib/Differentiator/ReverseModeVisitor.cpp	`95.47% <100.00%> (+0.02%)`	⬆️
lib/Differentiator/VisitorBase.cpp	`97.14% <100.00%> (+0.07%)`	⬆️

Files with missing lines	Coverage Δ
include/clad/Differentiator/VisitorBase.h	`100.00% <ø> (ø)`
lib/Differentiator/BaseForwardModeVisitor.cpp	`98.73% <100.00%> (+<0.01%)`	⬆️
lib/Differentiator/ReverseModeVisitor.cpp	`95.47% <100.00%> (+0.02%)`	⬆️
lib/Differentiator/VisitorBase.cpp	`97.14% <100.00%> (+0.07%)`	⬆️

vgvassilev · 2024-09-11T19:05:39Z

lib/Differentiator/VisitorBase.cpp

+
+    std::reverse(NNChain.begin(), NNChain.end());
+
+    for (auto& n : NNChain) {


I am wondering if we can't use (or draw inspiration from) https://github.com/llvm-mirror/clang/blob/aa231e4be75ac4759c236b755c57876f76e3cf05/lib/AST/QualTypeNames.cpp#L211

EDIT: That's the right link: https://github.com/llvm/llvm-project/blob/b06954a5d02a41a38b72f7914c791428ccd95318/clang/lib/AST/QualTypeNames.cpp#L214

I saw it but I didn't dive deep into reading it, since I'd thought it's unrelated, cos it's about type names only.

The routine is practically the same and more complete to what we are trying to do here. It’s not possible to call it though. Maybe we should at least add a fixme and refer to it as a potential place to steal code from. It has a lot of tests and presumably should work more reliably.

alright, a fixme sounds fair. I'll also try to maybe incorporate some more things from it

github-actions

clang-tidy made some suggestions

lib/Differentiator/VisitorBase.cpp

github-actions

clang-tidy made some suggestions

lib/Differentiator/ReverseModeVisitor.cpp

gojakuch · 2024-10-06T09:58:07Z

@vgvassilev thanks for the rebase! I'm still working on fixing the last check, seems that it hasn't been affected that much

gojakuch · 2024-10-06T20:56:08Z

@vgvassilev it finally passed all the checks, I'm just going to squash everything now

vgvassilev

Can you address the clang-tidy comments?

vgvassilev · 2024-10-07T02:53:02Z

test/ValidCodeGen/ValidCodeGen.C

@@ -0,0 +1,80 @@
+// XFAIL: asserts


Why do we fail in debug builds here?

I think I just forgot to remove this line. thanks for pointing it out. it should be fine, but let's wait for the checks to pass

oh, seems like it was actually doing something, now I remember. unfortunately, the debug build is very picky. I have a couple of FIXME messages in that place and once they are addressed the XFAIL line can be removed.

the problem is that inside Clang they're using locations to build these nested names, while I'm using other things here, since we don't really have valid locations to provide to those Clang methods (providing valid locations from the original source code doesn't seem to work here). my approach works for nested names with namespace and basic class names, but the debug build runs into some asserts if you try to extend a CXXScopeSpec with a class name this way.

I can remove the XFAIL and update the tests so that they don't contain a class name in the nested name specifier chain and then open another separate issue for this case. does that sound good? or we can just not flag the issue #1050 as solved in this PR and update it to say that it's only partially solved.

Maybe update the tests and remove the xfail flag. Then we can adjust them back when fixing the issue #1050.

@vgvassilev all done. the checks have passed

github-actions

clang-tidy made some suggestions

github-actions · 2024-10-07T15:27:00Z

lib/Differentiator/ReverseModeVisitor.cpp

@@ -2167,6 +2170,28 @@ Expr* getArraySizeExpr(const ArrayType* AT, ASTContext& context,
          utils::BuildMemberExpr(m_Sema, getCurrentScope(), callRes, "adjoint");
      return StmtDiff(resValue, resAdjoint, resAdjoint);
    } // Recreate the original call expression.
+
+    if (const auto* OCE = dyn_cast<CXXOperatorCallExpr>(CE)) {
+      auto* FD = const_cast<CXXMethodDecl*>(


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

auto* FD = const_cast<CXXMethodDecl*>( ^

This commit fixes the way Clad generates code. Specifically, it addresses the way operators appear in the generated code in the reverse mode and the way nested name qualifiers are built in both modes. This partially f ixes vgvassilev#1050. Fixes: vgvassilev#1087

vgvassilev

LGTM!

gojakuch marked this pull request as draft September 10, 2024 19:14

github-actions bot reviewed Sep 10, 2024

View reviewed changes

gojakuch marked this pull request as ready for review September 11, 2024 16:35

vgvassilev reviewed Sep 11, 2024

View reviewed changes

github-actions bot reviewed Sep 28, 2024

View reviewed changes

gojakuch force-pushed the issue-1050 branch from b53646a to 2c88aed Compare October 1, 2024 20:36

github-actions bot reviewed Oct 2, 2024

View reviewed changes

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

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

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

vgvassilev force-pushed the issue-1050 branch from 632f620 to 238286f Compare October 6, 2024 09:10

gojakuch force-pushed the issue-1050 branch from b37bac1 to 6fb1645 Compare October 6, 2024 21:01

vgvassilev reviewed Oct 7, 2024

View reviewed changes

gojakuch force-pushed the issue-1050 branch from 6fb1645 to c3c3aa6 Compare October 7, 2024 15:13

github-actions bot reviewed Oct 7, 2024

View reviewed changes

gojakuch force-pushed the issue-1050 branch 2 times, most recently from d47e2b7 to 82a6527 Compare October 8, 2024 08:07

gojakuch force-pushed the issue-1050 branch from 82a6527 to 8243b3c Compare October 8, 2024 08:08

vgvassilev approved these changes Oct 8, 2024

View reviewed changes

vgvassilev merged commit 4f2a378 into vgvassilev:master Oct 8, 2024
90 checks passed

gojakuch mentioned this pull request Oct 8, 2024

Clad is not building the nested name qualifiers correctly #1050

Open

gojakuch deleted the issue-1050 branch December 7, 2024 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the generation of invalid code in some common cases #1088

Fix the generation of invalid code in some common cases #1088

gojakuch commented Sep 10, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot Sep 10, 2024

codecov bot commented Sep 11, 2024 •

edited

Loading

vgvassilev Sep 11, 2024 •

edited

Loading

gojakuch Sep 11, 2024

vgvassilev Sep 11, 2024

gojakuch Sep 12, 2024

github-actions bot left a comment

github-actions bot left a comment

gojakuch commented Oct 6, 2024

gojakuch commented Oct 6, 2024

vgvassilev left a comment

vgvassilev Oct 7, 2024

gojakuch Oct 7, 2024

gojakuch Oct 7, 2024 •

edited

Loading

vgvassilev Oct 8, 2024

gojakuch Oct 8, 2024

github-actions bot left a comment

github-actions bot Oct 7, 2024

vgvassilev left a comment


		std::reverse(NNChain.begin(), NNChain.end());

		for (auto& n : NNChain) {

Fix the generation of invalid code in some common cases #1088

Fix the generation of invalid code in some common cases #1088

Conversation

gojakuch commented Sep 10, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Sep 10, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

vgvassilev Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

gojakuch Sep 11, 2024

Choose a reason for hiding this comment

vgvassilev Sep 11, 2024

Choose a reason for hiding this comment

gojakuch Sep 12, 2024

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

gojakuch commented Oct 6, 2024

gojakuch commented Oct 6, 2024

vgvassilev left a comment

Choose a reason for hiding this comment

vgvassilev Oct 7, 2024

Choose a reason for hiding this comment

gojakuch Oct 7, 2024

Choose a reason for hiding this comment

gojakuch Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

vgvassilev Oct 8, 2024

Choose a reason for hiding this comment

gojakuch Oct 8, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Oct 7, 2024

Choose a reason for hiding this comment

vgvassilev left a comment

Choose a reason for hiding this comment

gojakuch commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

vgvassilev Sep 11, 2024 •

edited

Loading

gojakuch Oct 7, 2024 •

edited

Loading