Re-order content in OMPDeclareReductionDecl dump

[ABataev]

This is wrong, the original implementation is correct and should not be changed.

[rsmith]

In D55395#1322244, @ABataev wrote:

This is wrong, the original implementation is correct and should not be changed.

The original implementation looks pretty clearly wrong to me, but I think this is wrong too. It looks like what was intended here was probably to label the two children as "combiner" and "initializer" respectively. As-is, the "combiner" text on the OMPDeclareReductionDecl line means nothing at all.

[ABataev]

In D55395#1322431, @rsmith wrote:

In D55395#1322244, @ABataev wrote:

This is wrong, the original implementation is correct and should not be changed.

The original implementation looks pretty clearly wrong to me, but I think this is wrong too. It looks like what was intended here was probably to label the two children as "combiner" and "initializer" respectively. As-is, the "combiner" text on the OMPDeclareReductionDecl line means nothing at all.

Oh yes, agree. Still must be reworked.

[steveire]

Use child node labels

[aaron.ballman]

This is a novel approach that's not used anywhere else in the AST dumper and there are several ways we could handle this, including:

• What's proposed (adding a new node to the tree that's not directly an AST node)
• Making use of the pointer information. e.g., https://pastebin.com/mh9dHT9L
• Adding the label before the AST node. e.g., https://pastebin.com/L8YwJTqe
• Adding the label after the AST node. e.g., https://pastebin.com/gbNahjsd
• Probably others

Why this way?

I'm not a huge fan of adding a new node to the tree that's not an AST node. It expands the tree both vertically (by adding a new node) and horizontally (by indenting everything below that new node) which I find visually distracting for the benefit provided. I personally prefer using the pointer information as it's less structurally disruptive and still provides the same information. I also find it a bit easier to match nodes up that way because the indentation level and tree-like adornments sometimes make it hard for me to determine relationships between when spatially far apart in the tree. There is precedence for using labels + pointers in the AST dumper already -- this is how we handle the prev and parent nodes for declarations, for instance.

If we're going to invent a novel way to do this, I do not think it should be done in an ad hoc manner, but should instead talk about whether we want to see this done in a more uniform fashion. For instance, how is this information any different than the list of overrides for a method, the previous declaration in a redecl, the parent of an out-of-line function definition, where a default template argument is inherited from, etc (all of which use pointers for relationships)? I don't feel the same way if we go with an existing practice that incrementally improves consistency.

[steveire]

In D55395#1324699, @aaron.ballman wrote:

This is a novel approach that's not used anywhere else in the AST dumper

Actually it's used by the InitListExpr dump. See https://reviews.llvm.org/D55488

[riccibruno]

In D55395#1324699, @aaron.ballman wrote:

This is a novel approach that's not used anywhere else in the AST dumper and there are several ways we could handle this, including:

• What's proposed (adding a new node to the tree that's not directly an AST node)
• Making use of the pointer information. e.g., https://pastebin.com/mh9dHT9L
• Adding the label before the AST node. e.g., https://pastebin.com/L8YwJTqe
• Adding the label after the AST node. e.g., https://pastebin.com/gbNahjsd
• Probably others

Why this way?

I'm not a huge fan of adding a new node to the tree that's not an AST node. It expands the tree both vertically (by adding a new node) and horizontally (by indenting everything below that new node) which I find visually distracting for the benefit provided. I personally prefer using the pointer information as it's less structurally disruptive and still provides the same information. I also find it a bit easier to match nodes up that way because the indentation level and tree-like adornments sometimes make it hard for me to determine relationships between when spatially far apart in the tree. There is precedence for using labels + pointers in the AST dumper already -- this is how we handle the prev and parent nodes for declarations, for instance.

If we're going to invent a novel way to do this, I do not think it should be done in an ad hoc manner, but should instead talk about whether we want to see this done in a more uniform fashion. For instance, how is this information any different than the list of overrides for a method, the previous declaration in a redecl, the parent of an out-of-line function definition, where a default template argument is inherited from, etc (all of which use pointers for relationships)? I don't feel the same way if we go with an existing practice that incrementally improves consistency.

I personally also prefer the first one. However is this really needed in this case ? The first sub-node will always be the combiner,
and then the second node will the the initializer if present. It seems to me that there is no ambiguity here.

[steveire]

I would prefer *not* to do what is in this patch. I'm just trying to find a way forward. I like the approach of using the existing precedent of printing the pointers as a compromise between removing such labels entirely and not having things like that at all. I did that for InitListExpr here: https://reviews.llvm.org/D55495

[steveire]

Use pointer approach

[aaron.ballman]

LGTM with a small nit. @ABataev, are you okay with this approach?

lib/AST/ASTDumper.cpp

1060 ↗

(On Diff #177428)

Elide braces and may as well make this const auto *. (If you want to hit the one above, that would also be good -- maybe even hoist the call into an initializer for a local variable to be used in both places?)

[ABataev]

lib/AST/ASTDumper.cpp

1056 ↗

(On Diff #177428)

Better to output it immediately after initializer keyword.

cfe/trunk/lib/AST/ASTDumper.cpp

void ASTDumper::VisitOMPThreadPrivateDecl(const OMPThreadPrivateDecl *D) {
   for (auto *E : D->varlists())
     dumpStmt(E);
 }
 
 void ASTDumper::VisitOMPDeclareReductionDecl(const OMPDeclareReductionDecl *D) {
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
   OS << " combiner";
-  dumpStmt(D->getCombiner());
-  if (auto *Initializer = D->getInitializer()) {
+  NodeDumper.dumpPointer(D->getCombiner());
+  if (const auto *Initializer = D->getInitializer()) {
     OS << " initializer";
+    NodeDumper.dumpPointer(Initializer);
     switch (D->getInitializerKind()) {
     case OMPDeclareReductionDecl::DirectInit:
       OS << " omp_priv = ";
       break;
     case OMPDeclareReductionDecl::CopyInit:
       OS << " omp_priv ()";
       break;
     case OMPDeclareReductionDecl::CallInit:
       break;
     }
-    dumpStmt(Initializer);
   }
+
+  dumpStmt(D->getCombiner());
+  if (const auto *Initializer = D->getInitializer())
+    dumpStmt(Initializer);
 }
 
 void ASTDumper::VisitOMPRequiresDecl(const OMPRequiresDecl *D) {
   for (auto *C : D->clauselists()) {
     dumpChild([=] {
       if (!C) {
         ColorScope Color(OS, ShowColors, NullColor);
         OS << "<<<NULL>>> OMPClause";

cfe/trunk/test/AST/dump.cpp

 // RUN: %clang_cc1 -verify -fopenmp -ast-dump %s | FileCheck %s
 // RUN: %clang_cc1 -verify -fopenmp-simd -ast-dump %s | FileCheck %s
 // expected-no-diagnostics
 
 int ga, gb;
 #pragma omp threadprivate(ga, gb)
 
 // CHECK:      |-OMPThreadPrivateDecl {{.+}} <col:9> col:9
 // CHECK-NEXT: | |-DeclRefExpr {{.+}} <col:27> 'int' lvalue Var {{.+}} 'ga' 'int'
 // CHECK-NEXT: | `-DeclRefExpr {{.+}} <col:31> 'int' lvalue Var {{.+}} 'gb' 'int'
 
 #pragma omp declare reduction(+ : int, char : omp_out *= omp_in)
 
 #pragma omp declare reduction(fun : float : omp_out += omp_in) initializer(omp_priv = omp_orig + 15)
 
-// CHECK:      |-OMPDeclareReductionDecl {{.+}} <line:[[@LINE-4]]:35> col:35 operator+ 'int' combiner
+// CHECK:      |-OMPDeclareReductionDecl {{.+}} <line:[[@LINE-4]]:35> col:35 operator+ 'int' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}} <col:47, col:58> 'int' lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}} <col:47> 'int' lvalue Var {{.+}} 'omp_out' 'int'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}} <col:58> 'int' <LValueToRValue>
 // CHECK-NEXT: | |   `-DeclRefExpr {{.+}} <col:58> 'int' lvalue Var {{.+}} 'omp_in' 'int'
 // CHECK-NEXT: | |-VarDecl {{.+}} <col:35> col:35 implicit used omp_in 'int'
 // CHECK-NEXT: | `-VarDecl {{.+}} <col:35> col:35 implicit used omp_out 'int'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}} <col:40> col:40 operator+ 'char' combiner
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}} <col:40> col:40 operator+ 'char' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}} <col:47, col:58> 'char' lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}} <col:47> 'char' lvalue Var {{.+}} 'omp_out' 'char'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}} <col:58> 'int' <IntegralCast>
 // CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}} <col:58> 'char' <LValueToRValue>
 // CHECK-NEXT: | |     `-DeclRefExpr {{.+}} <col:58> 'char' lvalue Var {{.+}} 'omp_in' 'char'
 // CHECK-NEXT: | |-VarDecl {{.+}} <col:40> col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}} <col:40> col:40 implicit used omp_out 'char'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}} <line:[[@LINE-17]]:37> col:37 fun 'float' combiner initializer
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}} <line:[[@LINE-17]]:37> col:37 fun 'float' combiner 0x{{.+}} initializer 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}} <col:45, col:56> 'float' lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}} <col:45> 'float' lvalue Var {{.+}} 'omp_out' 'float'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}} <col:56> 'float' <LValueToRValue>
 // CHECK-NEXT: | |   `-DeclRefExpr {{.+}} <col:56> 'float' lvalue Var {{.+}} 'omp_in' 'float'
 // CHECK-NEXT: | |-BinaryOperator {{.+}} <col:76, col:98> 'float' lvalue '='
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}} <col:76> 'float' lvalue Var {{.+}} 'omp_priv' 'float'
 // CHECK-NEXT: | | `-BinaryOperator {{.+}} <col:87, col:98> 'float' '+'
 // CHECK-NEXT: | |   |-ImplicitCastExpr {{.+}} <col:87> 'float' <LValueToRValue>