[SYCL] Fix bug with constexpr recursion

[elizabethandrews]

Commit 9a9a018 introduced a regression in following case -

E.g.

  struct ObjInit {
    int a;
  } objbar = {constexpr_recurse(1)};

The assert was hit when DeclStmt contains both TypeDecl and VarDecl i.e. a CXXRecordDecl which is not constexpr and a VarDecl which is. This PR removes code added in call graph creation. This code is not technically not required since the AST visitor in SemaSYCL, will handle non-traversal of nodes representing constexpr variables.

Closes: #4048

Signed-off-by: Elizabeth Andrews elizabeth.andrews@intel.com

[elizabethandrews]

Ideally, we could avoid creating the subgraphs for constexpr nodes as the removed code attempted to do.

I was however unable to figure out how to continue visiting Decl nodes which did not represent constexpr variables, since statement visitors handle only statements and expressions. I tried visiting initializers of these variables instead but ran into issues there as well. I can look into this in more detail if reviewers prefer not creating the subgraph.

[erichkeane]

Ideally, we could avoid creating the subgraphs for constexpr nodes as the removed code attempted to do.

I was however unable to figure out how to continue visiting Decl nodes which did not represent constexpr variables, since statement visitors handle only statements and expressions. I tried visiting initializers of these variables instead but ran into issues there as well. I can look into this in more detail if reviewers prefer not creating the subgraph.

What does the DeclStmt visit call by default? It should eventually call a VarDecl or something, right? Alternatively, we might have to loop through the decl-stmt children to find which are constexpr and skip those.

[elizabethandrews]

Ideally, we could avoid creating the subgraphs for constexpr nodes as the removed code attempted to do.
I was however unable to figure out how to continue visiting Decl nodes which did not represent constexpr variables, since statement visitors handle only statements and expressions. I tried visiting initializers of these variables instead but ran into issues there as well. I can look into this in more detail if reviewers prefer not creating the subgraph.

What does the DeclStmt visit call by default? It should eventually call a VarDecl or something, right? Alternatively, we might have to loop through the decl-stmt children to find which are constexpr and skip those.

I tried this after I debugged StmtVisitor::VisitDeclStmt. It calls S->children, but this returns the InitListExpr/ConstructorExpr, not the VarDecl. I wasn't sure how to tell if it was in a constexpr context or not.

[elizabethandrews]

Ideally, we could avoid creating the subgraphs for constexpr nodes as the removed code attempted to do.
I was however unable to figure out how to continue visiting Decl nodes which did not represent constexpr variables, since statement visitors handle only statements and expressions. I tried visiting initializers of these variables instead but ran into issues there as well. I can look into this in more detail if reviewers prefer not creating the subgraph.

What does the DeclStmt visit call by default? It should eventually call a VarDecl or something, right? Alternatively, we might have to loop through the decl-stmt children to find which are constexpr and skip those.

I tried this after I debugged StmtVisitor::VisitDeclStmt. It calls S->children, but this returns the InitListExpr/ConstructorExpr, not the VarDecl. I wasn't sure how to tell if it was called in constexpr context or not.

S->childen as in decl-stmt's children

[erichkeane]

Ideally, we could avoid creating the subgraphs for constexpr nodes as the removed code attempted to do.
I was however unable to figure out how to continue visiting Decl nodes which did not represent constexpr variables, since statement visitors handle only statements and expressions. I tried visiting initializers of these variables instead but ran into issues there as well. I can look into this in more detail if reviewers prefer not creating the subgraph.

What does the DeclStmt visit call by default? It should eventually call a VarDecl or something, right? Alternatively, we might have to loop through the decl-stmt children to find which are constexpr and skip those.

I tried this after I debugged StmtVisitor::VisitDeclStmt. It calls S->children, but this returns the InitListExpr/ConstructorExpr, not the VarDecl. I wasn't sure how to tell if it was called in constexpr context or not.

S->childen as in decl-stmt's children

childrenis a DeclGroupRef whose iterators are pointers-to-decl, so I'd imagine this must go through VisitDecl or something like that? Or presumably we could loop through and only visit the non-constexpr non-vardecls that way.

[elizabethandrews]

Ideally, we could avoid creating the subgraphs for constexpr nodes as the removed code attempted to do.
I was however unable to figure out how to continue visiting Decl nodes which did not represent constexpr variables, since statement visitors handle only statements and expressions. I tried visiting initializers of these variables instead but ran into issues there as well. I can look into this in more detail if reviewers prefer not creating the subgraph.

What does the DeclStmt visit call by default? It should eventually call a VarDecl or something, right? Alternatively, we might have to loop through the decl-stmt children to find which are constexpr and skip those.

I tried this after I debugged StmtVisitor::VisitDeclStmt. It calls S->children, but this returns the InitListExpr/ConstructorExpr, not the VarDecl. I wasn't sure how to tell if it was called in constexpr context or not.

S->childen as in decl-stmt's children

childrenis a DeclGroupRef whose iterators are pointers-to-decl, so I'd imagine this must go through VisitDecl or something like that? Or presumably we could loop through and only visit the non-constexpr non-vardecls that way.

This was one of the initial (incomplete) solutions I tried -

+  void VisitDecl(Decl *D) {
+    if (!D)
+      return;
+    if (VarDecl *VD = dyn_cast<VarDecl>(D))
+      if (VD->isConstexpr())
+         return;
+      else if (Expr *Init = VD->getInit())
+        VisitChildren(Init);
+  }
+
+  void VisitDeclStmt(DeclStmt *DS) {
+    if (G->shouldSkipConstantExpressions())
+      for (auto *D : DS->decls())
+        VisitDecl(D);
+    else
+      StmtVisitor::VisitDeclStmt(DS);

I think this is similar to one of the solutions you proposed? This caused lit failure which I believe was because VisitChildren(Init) is incorrect. Init itself needs to be visited and it wasn't immediately clear how since VisitStmt(Init) just calls VisitChildren. I did not debug this in detail. If you think this approach seems correct, I can look into this.

childrenis a DeclGroupRef whose iterators are pointers-to-decl, so I'd imagine this must go through VisitDecl or something like that?

Can you elaborate on this? You iterate through statements and visit sub-expressions, not Decls.

.../clang/lib/Analysis/CallGraph.cpp:166:35: error: cannot convert ‘clang::Stmt*’ to ‘clang::Decl*’ in initialization
       for (Decl *D : DS->children())

[erichkeane]

Ideally, we could avoid creating the subgraphs for constexpr nodes as the removed code attempted to do.
I was however unable to figure out how to continue visiting Decl nodes which did not represent constexpr variables, since statement visitors handle only statements and expressions. I tried visiting initializers of these variables instead but ran into issues there as well. I can look into this in more detail if reviewers prefer not creating the subgraph.

What does the DeclStmt visit call by default? It should eventually call a VarDecl or something, right? Alternatively, we might have to loop through the decl-stmt children to find which are constexpr and skip those.

I tried this after I debugged StmtVisitor::VisitDeclStmt. It calls S->children, but this returns the InitListExpr/ConstructorExpr, not the VarDecl. I wasn't sure how to tell if it was called in constexpr context or not.

S->childen as in decl-stmt's children

childrenis a DeclGroupRef whose iterators are pointers-to-decl, so I'd imagine this must go through VisitDecl or something like that? Or presumably we could loop through and only visit the non-constexpr non-vardecls that way.

This was one of the initial (incomplete) solutions I tried -

+  void VisitDecl(Decl *D) {
+    if (!D)
+      return;
+    if (VarDecl *VD = dyn_cast<VarDecl>(D))
+      if (VD->isConstexpr())
+         return;
+      else if (Expr *Init = VD->getInit())
+        VisitChildren(Init);
+  }
+
+  void VisitDeclStmt(DeclStmt *DS) {
+    if (G->shouldSkipConstantExpressions())
+      for (auto *D : DS->decls())
+        VisitDecl(D);
+    else
+      StmtVisitor::VisitDeclStmt(DS);

I think this is similar to one of the solutions you proposed? This caused lit failure which I believe was because VisitChildren(Init) is incorrect. Init itself needs to be visited and it wasn't immediately clear how since VisitStmt(Init) just calls VisitChildren. I did not debug this in detail. If you think this approach seems correct, I can look into this.

From my look (not having spent much time in this unfortuantely), that would be my guess, is something like that.

childrenis a DeclGroupRef whose iterators are pointers-to-decl, so I'd imagine this must go through VisitDecl or something like that?

Can you elaborate on this? You iterate through statements and visit sub-expressions, not Decls.

/export/iusers/eandrews/sandbox/publicsycl/llvm/clang/lib/Analysis/CallGraph.cpp:166:35: error: cannot convert ‘clang::Stmt*’ to ‘clang::Decl*’ in initialization
       for (Decl *D : DS->children())

Looks like DeclStmt::children is:
https://clang.llvm.org/doxygen/Stmt_8h_source.html#l01324

This appears to create an iterator to DG's (DeclGroupRef) begin/end, which are pointers to Decl. child_iterator is a StmtIterator whose Base contains a union of the stmt or Decl, and seems to know it is in DeclGroupMode. So it at least CONTAINS the declarations, though I don't have a good idea of what it DOES with them...

So at least the 'child' handler of it SHOULD have the information needed. Though it at least seems to go through GetDeclExpr, which gets the init of it if it is a VarDecl/in a DeclGroup.

I suspect something like the VisitDecl above is the right idea, but you'll likely want to loop through children/decls and find the corresponding ones in some way, so that you do a normal 'VisitStmt' on all children that AREN'T a constexpr VarDecl.

Check out StmtIteratorBase's NextDecl/HandleDecl for hints on how to keep the two iterator sets in sync.

[elizabethandrews]

Ideally, we could avoid creating the subgraphs for constexpr nodes as the removed code attempted to do.
I was however unable to figure out how to continue visiting Decl nodes which did not represent constexpr variables, since statement visitors handle only statements and expressions. I tried visiting initializers of these variables instead but ran into issues there as well. I can look into this in more detail if reviewers prefer not creating the subgraph.

What does the DeclStmt visit call by default? It should eventually call a VarDecl or something, right? Alternatively, we might have to loop through the decl-stmt children to find which are constexpr and skip those.

I tried this after I debugged StmtVisitor::VisitDeclStmt. It calls S->children, but this returns the InitListExpr/ConstructorExpr, not the VarDecl. I wasn't sure how to tell if it was called in constexpr context or not.

S->childen as in decl-stmt's children

childrenis a DeclGroupRef whose iterators are pointers-to-decl, so I'd imagine this must go through VisitDecl or something like that? Or presumably we could loop through and only visit the non-constexpr non-vardecls that way.

This was one of the initial (incomplete) solutions I tried -

+  void VisitDecl(Decl *D) {
+    if (!D)
+      return;
+    if (VarDecl *VD = dyn_cast<VarDecl>(D))
+      if (VD->isConstexpr())
+         return;
+      else if (Expr *Init = VD->getInit())
+        VisitChildren(Init);
+  }
+
+  void VisitDeclStmt(DeclStmt *DS) {
+    if (G->shouldSkipConstantExpressions())
+      for (auto *D : DS->decls())
+        VisitDecl(D);
+    else
+      StmtVisitor::VisitDeclStmt(DS);

I think this is similar to one of the solutions you proposed? This caused lit failure which I believe was because VisitChildren(Init) is incorrect. Init itself needs to be visited and it wasn't immediately clear how since VisitStmt(Init) just calls VisitChildren. I did not debug this in detail. If you think this approach seems correct, I can look into this.

From my look (not having spent much time in this unfortuantely), that would be my guess, is something like that.

childrenis a DeclGroupRef whose iterators are pointers-to-decl, so I'd imagine this must go through VisitDecl or something like that?

Can you elaborate on this? You iterate through statements and visit sub-expressions, not Decls.

/export/iusers/eandrews/sandbox/publicsycl/llvm/clang/lib/Analysis/CallGraph.cpp:166:35: error: cannot convert ‘clang::Stmt*’ to ‘clang::Decl*’ in initialization
       for (Decl *D : DS->children())

Looks like DeclStmt::children is: https://clang.llvm.org/doxygen/Stmt_8h_source.html#l01324

This appears to create an iterator to DG's (DeclGroupRef) begin/end, which are pointers to Decl. child_iterator is a StmtIterator whose Base contains a union of the stmt or Decl, and seems to know it is in DeclGroupMode. So it at least CONTAINS the declarations, though I don't have a good idea of what it DOES with them...

So at least the 'child' handler of it SHOULD have the information needed. Though it at least seems to go through GetDeclExpr, which gets the init of it if it is a VarDecl/in a DeclGroup.

I suspect something like the VisitDecl above is the right idea, but you'll likely want to loop through children/decls and find the corresponding ones in some way, so that you do a normal 'VisitStmt' on all children that AREN'T a constexpr VarDecl.

Check out StmtIteratorBase's NextDecl/HandleDecl for hints on how to keep the two iterator sets in sync.

Sorry for the VERY long delay in getting back to this PR. It slipped off my radar. I think I've implemented what you suggested here. The subgraph is not generated for constexpr nodes

[premanandrao]

I am okay with the changes. @erichkeane, since you were involved with the changes earlier, would you like to review as well?

[erichkeane]

I am okay with the changes. @erichkeane, since you were involved with the changes earlier, would you like to review as well?

I did a quick sweep yesterday and didn't have anything worth bringing up. Direction seems right (according to my memory), and your comments/questions seem to be productive enough that this looks thoroughly reviewed. I'm happy.

[elizabethandrews]

Thanks for the review @premanandrao and @erichkeane !

Patch has caused a crash in pre-commit testing though. I will investigate that and update PR.

[elizabethandrews]

Marking as Draft since the Visitor logic is flawed.

[elizabethandrews]

As per offline discussion, this PR now reverts problematic code.

[smanna12]

LGTM