[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 282682.
Tyker edited the summary of this revision.
Tyker added a comment.

Sorry for the delay

In D63640#2151016 , @rsmith wrote:

> Are we at a point where we can test this now?

Yes we can use consteval to test it. so i removed changes to constexpr AST 
modling.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/ASTMerge/APValue/APValue.cpp

Index: clang/test/ASTMerge/APValue/APValue.cpp
===
--- /dev/null
+++ clang/test/ASTMerge/APValue/APValue.cpp
@@ -0,0 +1,460 @@
+// RUN: %clang_cc1 -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -std=gnu++2a %s -DEMIT -ast-merge %t.pch -ast-dump-all | FileCheck %s
+
+#ifndef EMIT
+#define EMIT
+
+namespace Integer {
+
+consteval int fint() {
+  return 6789;
+}
+
+int Unique_Int = fint();
+//CHECK:  VarDecl {{.*}} Unique_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'int'
+//CHECK-NEXT: value: Int 6789
+
+consteval __uint128_t fint128() {
+  return ((__uint128_t)0x75f17d6b3588f843 << 64) | 0xb13dea7c9c324e51;
+}
+
+constexpr __uint128_t Unique_Int128 = fint128();
+//CHECK:  VarDecl {{.*}} Unique_Int128
+//CHECK-NEXT: value: Int 156773562844924187900898496343692168785
+//CHECK-NEXT: ConstantExpr
+//CHECK-NEXT: value: Int 156773562844924187900898496343692168785
+
+} // namespace Integer
+
+namespace FloatingPoint {
+
+consteval double fdouble() {
+  return double(567890.67890);
+}
+
+double Unique_Double = fdouble();
+//CHECK:  VarDecl {{.*}} Unique_Double
+//CHECK-NEXT: ConstantExpr {{.*}}
+//CHECK-NEXT: value: Float 5.678907e+05
+
+} // namespace FloatingPoint
+
+// FIXME: Add test for FixedPoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
+namespace Struct {
+
+struct B {
+  int i;
+  double d;
+};
+
+consteval B fB() {
+  return B{1, 0.7};
+}
+
+constexpr B Basic_Struct = fB();
+//CHECK:  VarDecl {{.*}} Basic_Struct
+//CHECK-NEXT: value: Struct
+//CHECK-NEXT: fields: Int 1, Float 7.00e-01
+//CHECK-NEXT: ImplicitCastExpr
+//CHECK-NEXT: ConstantExpr
+//CHECK-NEXT: value: Struct
+//CHECK-NEXT: fields: Int 1, Float 7.00e-01
+
+struct C {
+  int i = 9;
+};
+
+struct A : B {
+  constexpr A(B b, int I, double D, C _c) : B(b), i(I), d(D), c(_c) {}
+  int i;
+  double d;
+  C c;
+};
+
+consteval A fA() {
+  return A(Basic_Struct, 1, 79.789, {});
+}
+
+A Advanced_Struct = fA();
+//CHECK:  VarDecl {{.*}} Advanced_Struct
+//CHECK-NEXT: ConstantExpr {{.*}}
+//CHECK-NEXT: value: Struct
+//CHECK-NEXT: base: Struct
+//CHECK-NEXT: fields: Int 1, Float 7.00e-01
+//CHECK-NEXT: fields: Int 1, Float 7.978900e+01
+//CHECK-NEXT: field: Struct
+//CHECK-NEXT: field: Int 9
+
+} // namespace Struct
+
+namespace Vector {
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+consteval v4si fv4si() {
+  return (v4si){8, 2, 3};
+}
+
+v4si Vector_Int = fv4si();
+//CHECK:  VarDecl {{.*}} Vector_Int
+//CHECK-NEXT: ConstantExpr
+//CHECK-NEXT: value: Vector length=4
+//CHECK-NEXT: elements: Int 8, Int 2, Int 3, Int 0
+
+} // namespace Vector
+
+namespace Array {
+
+struct B {
+  int arr[6];
+};
+
+consteval B fint() {
+  return B{1, 2, 3, 4, 5, 6};
+}
+
+B Array_Int = fint();
+//CHECK:  VarDecl {{.*}} Array_Int
+//CHECK-NEXT: ConstantExpr
+//CHECK-NEXT: value: Struct
+//CHECK-NEXT: field: Array size=6
+//CHECK-NEXT: elements: Int 1, Int 2, Int 3, Int 4
+//CHECK-NEXT: elements: Int 5, Int 6
+
+struct A {
+  int i = 789;
+  double d = 67890.09876;
+};
+
+struct C {
+  A arr[3];
+};
+
+consteval C fA() {
+  return {{A{}, A{-45678, 9.8}, A{9}}};
+}
+
+C Array2_Struct = fA();
+//CHECK:  VarDecl {{.*}} Array2_Struct
+//CHECK-NEXT: ConstantExpr {{.*}}
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+struct D {
+  v4si arr[2];
+};
+
+consteval D fv4si() {
+  return {{{1, 2, 3, 4}, {4, 5, 6, 7}}};
+}
+
+D Array_Vector = fv4si();
+//CHECK:  VarDecl {{.*}} Array_Vector
+//CHECK-NEXT: ConstantExpr {{.*}}
+//CHECK-NEXT: value: Struct
+//CHECK-NEXT: field: Array size=2
+//CHECK-NEXT: element: Vector length=4
+//CHECK-NEXT: elements: Int 1, Int 2, Int 3, Int 4
+//CHECK-NEXT: element: Vector length=4
+//CHECK-NEXT: elements: Int 4, Int 5, Int 6, Int 7
+
+} // namespace Array
+
+namespace Union {
+
+struct A {
+  int i = 6789;
+  float f = 987.9876;
+};
+
+union U {
+  int i;
+  A a{567890, 9876.5678f};
+};
+
+consteval U fU1() {
+  return U{0};
+}
+
+U Unique_Union1 = fU1();
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr
+//CHECK-NEXT: value: Union .i Int 0
+
+consteval U fU() {
+  return U{};
+}
+
+U 

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Are we at a point where we can test this now? Perhaps by adding an assert in 
codegen that we always have an evaluated value for any `constexpr` variable 
that we emit?




Comment at: clang/lib/Sema/SemaDecl.cpp:11883-11885
   ExprResult Result =
   ActOnFinishFullExpr(Init, VDecl->getLocation(),
   /*DiscardedValue*/ false, VDecl->isConstexpr());

This may create additional AST nodes outside the `ConstantExpr`, which 
`VarDecl::evaluateValue` is not expecting to see (in particular, if we have 
cleanups for the initializer). Should the `ConstantExpr` go outside those nodes 
rather than inside?



Comment at: clang/lib/Serialization/ASTReader.cpp:9635
+if (IsExpr) {
+  Base = APValue::LValueBase(ReadExpr(F), CallIndex, Version);
+  ElemTy = Base.get()->getType();

Tyker wrote:
> Tyker wrote:
> > rsmith wrote:
> > > This is problematic.
> > > 
> > > `ReadExpr` will read a new copy of the expression, creating a distinct 
> > > object. But in the case where we reach this when deserializing (for a 
> > > `MaterializeTemporaryExpr`), we need to refer to the existing 
> > > `MaterializeTemporaryExpr` in the initializer of its lifetime-extending 
> > > declaration. We will also need to serialize the `ASTContext`'s 
> > > `MaterializedTemporaryValues` collection so that the temporaries 
> > > lifetime-extended in a constant initializer get properly handled.
> > > 
> > > That all sounds very messy, so I think we should reconsider the model 
> > > that we use for lifetime-extended materialized temporaries. As a 
> > > half-baked idea:
> > > 
> > >  * When we lifetime-extend a temporary, create a 
> > > `MaterializedTemporaryDecl` to hold its value, and modify 
> > > `MaterializeTemporaryExpr` to refer to the `MaterializedTemporaryDecl` 
> > > rather than to just hold the subexpression for the temporary.
> > >  * Change the `LValueBase` representation to denote the declaration 
> > > rather than the expression.
> > >  * Store the constant evaluated value for a materialized temporary on the 
> > > `MaterializedTemporaryDecl` rather than on a side-table in the 
> > > `ASTContext`.
> > > 
> > > With that done, we should verify that all remaining `Expr*`s used as 
> > > `LValueBase`s are either only transiently used during evaluation or don't 
> > > have these kinds of identity problems.
> > Would it be possible to adapt serialization/deserialization so that they 
> > make sure that `MaterializeTemporaryExpr` are unique.
> > by:
> > 
> >   - When serializing `MaterializeTemporaryExpr` serialize a key obtained 
> > from the pointer to the expression as it is unique.
> >   - When deserializing `MaterializeTemporaryExpr` deserializing the key, 
> > and than have a cache for previously deserialized expression that need to 
> > be unique.
> > 
> > This would make easier adding new `Expr` that require uniqueness and seem 
> > less complicated.
> > What do you think ?
> i added a review that does the refactoring https://reviews.llvm.org/D69360.
What are the cases for which we still encounter expressions as lvalue bases 
during serialization? I think all the other ones should be OK, but maybe 
there's another interesting one we've overlooked.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 236707.
Tyker added a comment.

rebased


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640

Files:
  clang/include/clang/AST/APValue.h
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTImporter.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/PrettyPrinter.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/AST/ast-dump-color.cpp

Index: clang/test/AST/ast-dump-color.cpp
===
--- clang/test/AST/ast-dump-color.cpp
+++ clang/test/AST/ast-dump-color.cpp
@@ -49,13 +49,13 @@
 //CHECK: {{^}}[[Blue]]| |   |-[[RESET]][[MAGENTA]]IntegerLiteral[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:10:11[[RESET]]> [[Green]]'int'[[RESET]][[Cyan:.\[0;36m]][[RESET]][[Cyan]][[RESET]][[CYAN]] 1[[RESET]]{{$}}
 //CHECK: {{^}}[[Blue]]| |   `-[[RESET]][[MAGENTA]]CompoundStmt[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:14[[RESET]], [[Yellow]]line:15:3[[RESET]]>{{$}}
 //CHECK: {{^}}[[Blue]]| | |-[[RESET]][[MAGENTA]]CaseStmt[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:11:3[[RESET]], [[Yellow]]line:12:27[[RESET]]>{{$}}
-//CHECK: {{^}}[[Blue]]| | | |-[[RESET]][[MAGENTA]]ConstantExpr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:11:8[[RESET]]> [[Green]]'int'[[RESET]][[Cyan]][[RESET]][[Cyan]][[RESET]][[CYAN]] Int: 1[[RESET]]{{$}}
+//CHECK: {{^}}[[Blue]]| | | |-[[RESET]][[MAGENTA]]ConstantExpr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:11:8[[RESET]]> [[Green]]'int'[[RESET]][[Cyan]][[RESET]][[Cyan]][[RESET]][[CYAN]] 1[[RESET]]{{$}}
 //CHECK: {{^}}[[Blue]]| | | | `-[[RESET]][[MAGENTA]]IntegerLiteral[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:8[[RESET]]> [[Green]]'int'[[RESET]][[Cyan]][[RESET]][[Cyan]][[RESET]][[CYAN]] 1[[RESET]]{{$}}
 //CHECK: {{^}}[[Blue]]| | | `-[[RESET]][[MAGENTA]]AttributedStmt[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:12:5[[RESET]], [[Yellow]]col:27[[RESET]]>{{$}}
 //CHECK: {{^}}[[Blue]]| | |   |-[[RESET]][[BLUE]]FallThroughAttr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:7[[RESET]], [[Yellow]]col:14[[RESET]]>{{$}}
 //CHECK: {{^}}[[Blue]]| | |   `-[[RESET]][[MAGENTA]]NullStmt[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:27[[RESET]]>{{$}}
 //CHECK: {{^}}[[Blue]]| | `-[[RESET]][[MAGENTA]]CaseStmt[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:13:3[[RESET]], [[Yellow]]line:14:5[[RESET]]>{{$}}
-//CHECK: {{^}}[[Blue]]| |   |-[[RESET]][[MAGENTA]]ConstantExpr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:13:8[[RESET]]> [[Green]]'int'[[RESET]][[Cyan]][[RESET]][[Cyan]][[RESET]][[CYAN]] Int: 2[[RESET]]{{$}}
+//CHECK: {{^}}[[Blue]]| |   |-[[RESET]][[MAGENTA]]ConstantExpr[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:13:8[[RESET]]> [[Green]]'int'[[RESET]][[Cyan]][[RESET]][[Cyan]][[RESET]][[CYAN]] 2[[RESET]]{{$}}
 //CHECK: {{^}}[[Blue]]| |   | `-[[RESET]][[MAGENTA]]IntegerLiteral[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]col:8[[RESET]]> [[Green]]'int'[[RESET]][[Cyan]][[RESET]][[Cyan]][[RESET]][[CYAN]] 2[[RESET]]{{$}}
 //CHECK: {{^}}[[Blue]]| |   `-[[RESET]][[MAGENTA]]NullStmt[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:14:5[[RESET]]>{{$}}
 //CHECK: {{^}}[[Blue]]| `-[[RESET]][[Blue]]FullComment[[RESET]][[Yellow]] 0x{{[0-9a-fA-F]*}}[[RESET]] <[[Yellow]]line:8:4[[RESET]], [[Yellow]]col:11[[RESET]]>{{$}}
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -473,7 +473,8 @@
   case ConstantExpr::RSK_Int64:
 Record.push_back(E->Int64Result());
 Record.push_back(E->ConstantExprBits.IsUnsigned |
- E->ConstantExprBits.BitWidth << 1);
+ E->ConstantExprBits.BitWidth << 1 |
+ E->ConstantExprBits.APValueKind << 8);
 break;
   case ConstantExpr::RSK_APValue:
 Record.AddAPValue(E->APValueResult());
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -5065,14 +5065,90 @@
 AddAPFloat(Value.getComplexFloatImag());
 return;
   }
-  case APValue::LValue:
   case APValue::Vector:
+push_back(Value.getVectorLength());
+for (unsigned Idx = 0; Idx < 

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

now that the issue with uniqueness of expressions is solved. we should be able 
to keep going on that review @rsmith.
https://reviews.llvm.org/D63960 should be i think close to completion. so maybe 
for testing i could use immediate invocation as a source for ConstantExpr 
instead of the code i added to make constexpr variables emit ConstantExpr ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker marked an inline comment as done.
Tyker added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:9635
+if (IsExpr) {
+  Base = APValue::LValueBase(ReadExpr(F), CallIndex, Version);
+  ElemTy = Base.get()->getType();

Tyker wrote:
> rsmith wrote:
> > This is problematic.
> > 
> > `ReadExpr` will read a new copy of the expression, creating a distinct 
> > object. But in the case where we reach this when deserializing (for a 
> > `MaterializeTemporaryExpr`), we need to refer to the existing 
> > `MaterializeTemporaryExpr` in the initializer of its lifetime-extending 
> > declaration. We will also need to serialize the `ASTContext`'s 
> > `MaterializedTemporaryValues` collection so that the temporaries 
> > lifetime-extended in a constant initializer get properly handled.
> > 
> > That all sounds very messy, so I think we should reconsider the model that 
> > we use for lifetime-extended materialized temporaries. As a half-baked idea:
> > 
> >  * When we lifetime-extend a temporary, create a 
> > `MaterializedTemporaryDecl` to hold its value, and modify 
> > `MaterializeTemporaryExpr` to refer to the `MaterializedTemporaryDecl` 
> > rather than to just hold the subexpression for the temporary.
> >  * Change the `LValueBase` representation to denote the declaration rather 
> > than the expression.
> >  * Store the constant evaluated value for a materialized temporary on the 
> > `MaterializedTemporaryDecl` rather than on a side-table in the `ASTContext`.
> > 
> > With that done, we should verify that all remaining `Expr*`s used as 
> > `LValueBase`s are either only transiently used during evaluation or don't 
> > have these kinds of identity problems.
> Would it be possible to adapt serialization/deserialization so that they make 
> sure that `MaterializeTemporaryExpr` are unique.
> by:
> 
>   - When serializing `MaterializeTemporaryExpr` serialize a key obtained from 
> the pointer to the expression as it is unique.
>   - When deserializing `MaterializeTemporaryExpr` deserializing the key, and 
> than have a cache for previously deserialized expression that need to be 
> unique.
> 
> This would make easier adding new `Expr` that require uniqueness and seem 
> less complicated.
> What do you think ?
i added a review that does the refactoring https://reviews.llvm.org/D69360.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker added a comment.

ping @rsmith


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker marked 10 inline comments as done.
Tyker added a comment.

update done tasks.




Comment at: clang/lib/AST/APValue.cpp:599
 Out << '[' << Path[I].getAsArrayIndex() << ']';
-ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+ElemTy = cast(ElemTy)->getElementType();
   }

aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > Are you sure this doesn't change behavior? See the implementation of 
> > > `ASTContext::getAsArrayType()`. Same question applies below.
> > i ran the test suite after the change it there wasn't any test failures. 
> > but the test on dumping APValue are probably not as thorough as we would 
> > like them to be.
> > from analysis of `ASTContext::getAsArrayType()` the only effect i see on 
> > the element type is de-sugaring and canonicalization which shouldn't affect 
> > correctness of the output.  de-sugaring requires the ASTContext but 
> > canonicalization doesn't.
> > 
> > i think the best way the have higher confidence is to ask rsmith what he 
> > thinks.
> Yeah, I doubt we have good test coverage for all the various behaviors here. 
> I was wondering if the qualifiers bit was handled properly with a simple 
> cast. @rsmith is a good person to weigh in.
the original question we had is whether it is correct to replace 
`Ctx.ASTContext::getAsArrayType(ElemTy)` by 
`cast(ElemTy.getCanonicalType())` in this context and the other 
comment below.



Comment at: clang/lib/AST/Expr.cpp:319
   case RSK_None:
 return;
   case RSK_Int64:

rsmith wrote:
> Can you use `llvm_unreachable` here? (Are there cases where we use `RSK_None` 
> and then later find we actually have a value to store into the 
> `ConstantExpr`?)
we can put `llvm_unreachable` in the switch because of `if (!Value.hasValue())` 
above the switch but we can't remove `if (!Value.hasValue())`.
all cases i have seen where `if (!Value.hasValue())` is taken occur after a 
semantic error occured. 



Comment at: clang/lib/Serialization/ASTReader.cpp:9635
+if (IsExpr) {
+  Base = APValue::LValueBase(ReadExpr(F), CallIndex, Version);
+  ElemTy = Base.get()->getType();

rsmith wrote:
> This is problematic.
> 
> `ReadExpr` will read a new copy of the expression, creating a distinct 
> object. But in the case where we reach this when deserializing (for a 
> `MaterializeTemporaryExpr`), we need to refer to the existing 
> `MaterializeTemporaryExpr` in the initializer of its lifetime-extending 
> declaration. We will also need to serialize the `ASTContext`'s 
> `MaterializedTemporaryValues` collection so that the temporaries 
> lifetime-extended in a constant initializer get properly handled.
> 
> That all sounds very messy, so I think we should reconsider the model that we 
> use for lifetime-extended materialized temporaries. As a half-baked idea:
> 
>  * When we lifetime-extend a temporary, create a `MaterializedTemporaryDecl` 
> to hold its value, and modify `MaterializeTemporaryExpr` to refer to the 
> `MaterializedTemporaryDecl` rather than to just hold the subexpression for 
> the temporary.
>  * Change the `LValueBase` representation to denote the declaration rather 
> than the expression.
>  * Store the constant evaluated value for a materialized temporary on the 
> `MaterializedTemporaryDecl` rather than on a side-table in the `ASTContext`.
> 
> With that done, we should verify that all remaining `Expr*`s used as 
> `LValueBase`s are either only transiently used during evaluation or don't 
> have these kinds of identity problems.
Would it be possible to adapt serialization/deserialization so that they make 
sure that `MaterializeTemporaryExpr` are unique.
by:

  - When serializing `MaterializeTemporaryExpr` serialize a key obtained from 
the pointer to the expression as it is unique.
  - When deserializing `MaterializeTemporaryExpr` deserializing the key, and 
than have a cache for previously deserialized expression that need to be unique.

This would make easier adding new `Expr` that require uniqueness and seem less 
complicated.
What do you think ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/AST/Expr.cpp:319
   case RSK_None:
 return;
   case RSK_Int64:

Can you use `llvm_unreachable` here? (Are there cases where we use `RSK_None` 
and then later find we actually have a value to store into the `ConstantExpr`?)



Comment at: clang/lib/Serialization/ASTReader.cpp:9635
+if (IsExpr) {
+  Base = APValue::LValueBase(ReadExpr(F), CallIndex, Version);
+  ElemTy = Base.get()->getType();

This is problematic.

`ReadExpr` will read a new copy of the expression, creating a distinct object. 
But in the case where we reach this when deserializing (for a 
`MaterializeTemporaryExpr`), we need to refer to the existing 
`MaterializeTemporaryExpr` in the initializer of its lifetime-extending 
declaration. We will also need to serialize the `ASTContext`'s 
`MaterializedTemporaryValues` collection so that the temporaries 
lifetime-extended in a constant initializer get properly handled.

That all sounds very messy, so I think we should reconsider the model that we 
use for lifetime-extended materialized temporaries. As a half-baked idea:

 * When we lifetime-extend a temporary, create a `MaterializedTemporaryDecl` to 
hold its value, and modify `MaterializeTemporaryExpr` to refer to the 
`MaterializedTemporaryDecl` rather than to just hold the subexpression for the 
temporary.
 * Change the `LValueBase` representation to denote the declaration rather than 
the expression.
 * Store the constant evaluated value for a materialized temporary on the 
`MaterializedTemporaryDecl` rather than on a side-table in the `ASTContext`.

With that done, we should verify that all remaining `Expr*`s used as 
`LValueBase`s are either only transiently used during evaluation or don't have 
these kinds of identity problems.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 222169.
Tyker marked 3 inline comments as done.
Tyker added a comment.

made renamings


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640

Files:
  clang/include/clang/AST/APValue.h
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTImporter.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/PrettyPrinter.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/AST/ast-dump-color.cpp
  clang/test/ASTMerge/APValue/APValue.cpp

Index: clang/test/ASTMerge/APValue/APValue.cpp
===
--- /dev/null
+++ clang/test/ASTMerge/APValue/APValue.cpp
@@ -0,0 +1,209 @@
+// RUN: %clang_cc1 -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -std=gnu++2a %s -DEMIT -ast-merge %t.pch -ast-dump-all | FileCheck %s
+
+#ifndef EMIT
+#define EMIT
+
+namespace Integer {
+
+constexpr int Unique_Int = int(6789);
+//CHECK:  VarDecl {{.*}} Unique_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'int' 6789
+
+constexpr __uint128_t Unique_Int128 = ((__uint128_t)0x75f17d6b3588f843 << 64) | 0xb13dea7c9c324e51;
+//CHECK:  VarDecl {{.*}} Unique_Int128 
+//CHECK-NEXT: ConstantExpr {{.*}} 'unsigned __int128' 156773562844924187900898496343692168785
+
+}
+
+namespace FloatingPoint {
+
+constexpr double Unique_Double = double(567890.67890);
+//CHECK:  VarDecl {{.*}} Unique_Double
+//CHECK-NEXT: ConstantExpr {{.*}} 'double' 5.678907e+05
+
+}
+
+// FIXME: Add test for FixedPoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
+namespace Struct {
+
+struct B {
+  int i;
+  double d;
+};
+
+constexpr B Basic_Struct = B{1, 0.7};
+//CHECK:  VarDecl {{.*}} Basic_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::B' {1, 7.00e-01}
+
+struct C {
+  int i = 9;
+};
+
+struct A : B {
+  int i;
+  double d;
+  C c;
+};
+
+constexpr A Advanced_Struct = A{Basic_Struct, 1, 79.789, {}};
+//CHECK:  VarDecl {{.*}} Advanced_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::A' {{[{][{]}}1, 7.00e-01}, 1, 7.978900e+01, {9}}
+
+}
+
+namespace Vector {
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Vector_Int = (v4si){8, 2, 3};
+//CHECK:  VarDecl {{.*}} Vector_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'Vector::v4si':'__attribute__((__vector_size__(4 * sizeof(int int' {8, 2, 3, 0}
+
+}
+
+namespace Array {
+
+constexpr int Array_Int[] = {1, 2, 3, 4, 5, 6};
+//CHECK:  VarDecl {{.*}} Array_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int [6]' {1, 2, 3, 4, 5, 6}
+
+struct A {
+  int i = 789;
+  double d = 67890.09876;
+};
+
+constexpr A Array2_Struct[][3] = {{{}, {-45678, 9.8}, {9}}, {{}}};
+//CHECK:  VarDecl {{.*}} Array2_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'Array::A const [2][3]' {{[{][{]}}{789, 6.789010e+04}, {-45678, 9.80e+00}, {9, 6.789010e+04}}, {{[{][{]}}789, 6.789010e+04}, {789, 6.789010e+04}, {789, 6.789010e+04}}}
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Array_Vector[] = {{1, 2, 3, 4}, {4, 5, 6, 7}};
+//CHECK:  VarDecl {{.*}} Array_Vector
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Array::v4si [2]' {{[{][{]}}1, 2, 3, 4}, {4, 5, 6, 7}}
+
+}
+
+namespace Union {
+
+struct A {
+  int i = 6789;
+  float f = 987.9876;
+};
+
+union U {
+  int i;
+  A a{567890, 9876.5678f};
+};
+
+constexpr U Unique_Union1 = U{0};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.i = 0}
+
+constexpr U Unique_Union2 = U{};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.a = {567890, 9.876567e+03}}
+
+}
+
+namespace MemberPointer{
+
+struct A {
+  struct B {
+struct C {
+  struct D {
+struct E {
+  struct F {
+struct G {
+  int i;
+};
+  };
+};
+  };
+};
+  };
+};
+
+constexpr auto MemberPointer1 = ::B::C::D::E::F::G::i;
+//CHECK:  VarDecl {{.*}} MemberPointer1
+//CHECK-NEXT: ConstantExpr {{.*}} 'int MemberPointer::A::B::C::D::E::F::G::*' ::i
+
+struct A1 {
+  struct B1 {
+int f() const {
+  return 0;
+}
+  };
+
+};
+
+constexpr auto MemberPointer2 = ::B1::f;
+//CHECK:  VarDecl {{.*}} MemberPointer2
+//CHECK-NEXT: ConstantExpr {{.*}} 'int (MemberPointer::A1::B1::*)() const' ::f
+
+}
+
+namespace std {
+  struct type_info;
+};
+
+namespace LValue {
+
+constexpr int LValueInt = 0;
+constexpr const int& ConstIntRef = LValueInt;
+//CHECK:  VarDecl {{.*}} ConstIntRef
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int' lvalue 
+constexpr 

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/AST/APValue.h:613
+  /// in place as after importing/deserializating then.
+  void reserveVector(unsigned N) {
+assert(isVector() && "Invalid accessor");

`ReserveVector`



Comment at: clang/include/clang/AST/APValue.h:620
+  unsigned Size);
+  void setLValueEmptyPath(LValueBase B, const CharUnits , unsigned Size,
+  bool OnePastTheEnd, bool IsNullPtr);

`SetLValueEmptyPath`



Comment at: clang/lib/AST/APValue.cpp:599
 Out << '[' << Path[I].getAsArrayIndex() << ']';
-ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+ElemTy = cast(ElemTy)->getElementType();
   }

Tyker wrote:
> aaron.ballman wrote:
> > Are you sure this doesn't change behavior? See the implementation of 
> > `ASTContext::getAsArrayType()`. Same question applies below.
> i ran the test suite after the change it there wasn't any test failures. but 
> the test on dumping APValue are probably not as thorough as we would like 
> them to be.
> from analysis of `ASTContext::getAsArrayType()` the only effect i see on the 
> element type is de-sugaring and canonicalization which shouldn't affect 
> correctness of the output.  de-sugaring requires the ASTContext but 
> canonicalization doesn't.
> 
> i think the best way the have higher confidence is to ask rsmith what he 
> thinks.
Yeah, I doubt we have good test coverage for all the various behaviors here. I 
was wondering if the qualifiers bit was handled properly with a simple cast. 
@rsmith is a good person to weigh in.



Comment at: clang/test/ASTMerge/APValue/APValue.cpp:28
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+

Tyker wrote:
> aaron.ballman wrote:
> > Tyker wrote:
> > > aaron.ballman wrote:
> > > > Are you planning to address this in this patch? Also, I think it's 
> > > > FixedPoint and not FixePoint.
> > > i don't intend to add them in this patch or subsequent patches. i don't 
> > > know how to use the features that have these representations, i don't 
> > > even know if they can be stored stored in that AST. so this is untested 
> > > code.
> > > that said theses representations aren't complex. the imporing for 
> > > FixePoint, ComplexInt, ComplexFloat is a no-op and for AddrLabelDiff it 
> > > is trivial. for serialization, I can put an llvm_unreachable to mark them 
> > > as untested if you want ?
> > I don't think `llvm_unreachable` makes a whole lot of sense unless the code 
> > is truly unreachable because there's no way to get an AST with that 
> > representation. By code inspection, the code looks reasonable but it does 
> > make me a bit uncomfortable to adopt it without tests. I suppose the FIXME 
> > is a reasonable compromise in this case, but if you have some spare cycles 
> > to investigate ways to get those representations, it would be appreciated.
> the reason i proposed `llvm_unreachable` was because it passes the tests and 
> prevents future developer from depending on the code that depend on it 
> assuming it works.
We typically only use `llvm_unreachable` for situations where we believe the 
code path is impossible to reach, which is why I think that's the wrong 
approach. We could use an assertion to test the theory, however.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.



Comment at: clang/lib/AST/APValue.cpp:599
 Out << '[' << Path[I].getAsArrayIndex() << ']';
-ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+ElemTy = cast(ElemTy)->getElementType();
   }

aaron.ballman wrote:
> Are you sure this doesn't change behavior? See the implementation of 
> `ASTContext::getAsArrayType()`. Same question applies below.
i ran the test suite after the change it there wasn't any test failures. but 
the test on dumping APValue are probably not as thorough as we would like them 
to be.
from analysis of `ASTContext::getAsArrayType()` the only effect i see on the 
element type is de-sugaring and canonicalization which shouldn't affect 
correctness of the output.  de-sugaring requires the ASTContext but 
canonicalization doesn't.

i think the best way the have higher confidence is to ask rsmith what he thinks.



Comment at: clang/test/ASTMerge/APValue/APValue.cpp:28
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+

aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > Are you planning to address this in this patch? Also, I think it's 
> > > FixedPoint and not FixePoint.
> > i don't intend to add them in this patch or subsequent patches. i don't 
> > know how to use the features that have these representations, i don't even 
> > know if they can be stored stored in that AST. so this is untested code.
> > that said theses representations aren't complex. the imporing for 
> > FixePoint, ComplexInt, ComplexFloat is a no-op and for AddrLabelDiff it is 
> > trivial. for serialization, I can put an llvm_unreachable to mark them as 
> > untested if you want ?
> I don't think `llvm_unreachable` makes a whole lot of sense unless the code 
> is truly unreachable because there's no way to get an AST with that 
> representation. By code inspection, the code looks reasonable but it does 
> make me a bit uncomfortable to adopt it without tests. I suppose the FIXME is 
> a reasonable compromise in this case, but if you have some spare cycles to 
> investigate ways to get those representations, it would be appreciated.
the reason i proposed `llvm_unreachable` was because it passes the tests and 
prevents future developer from depending on the code that depend on it assuming 
it works.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 221568.
Tyker marked 7 inline comments as done.
Tyker added a comment.

fixed most comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640

Files:
  clang/include/clang/AST/APValue.h
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTImporter.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/PrettyPrinter.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/AST/ast-dump-color.cpp
  clang/test/ASTMerge/APValue/APValue.cpp

Index: clang/test/ASTMerge/APValue/APValue.cpp
===
--- /dev/null
+++ clang/test/ASTMerge/APValue/APValue.cpp
@@ -0,0 +1,209 @@
+// RUN: %clang_cc1 -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -std=gnu++2a %s -DEMIT -ast-merge %t.pch -ast-dump-all | FileCheck %s
+
+#ifndef EMIT
+#define EMIT
+
+namespace Integer {
+
+constexpr int Unique_Int = int(6789);
+//CHECK:  VarDecl {{.*}} Unique_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'int' 6789
+
+constexpr __uint128_t Unique_Int128 = ((__uint128_t)0x75f17d6b3588f843 << 64) | 0xb13dea7c9c324e51;
+//CHECK:  VarDecl {{.*}} Unique_Int128 
+//CHECK-NEXT: ConstantExpr {{.*}} 'unsigned __int128' 156773562844924187900898496343692168785
+
+}
+
+namespace FloatingPoint {
+
+constexpr double Unique_Double = double(567890.67890);
+//CHECK:  VarDecl {{.*}} Unique_Double
+//CHECK-NEXT: ConstantExpr {{.*}} 'double' 5.678907e+05
+
+}
+
+// FIXME: Add test for FixedPoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
+namespace Struct {
+
+struct B {
+  int i;
+  double d;
+};
+
+constexpr B Basic_Struct = B{1, 0.7};
+//CHECK:  VarDecl {{.*}} Basic_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::B' {1, 7.00e-01}
+
+struct C {
+  int i = 9;
+};
+
+struct A : B {
+  int i;
+  double d;
+  C c;
+};
+
+constexpr A Advanced_Struct = A{Basic_Struct, 1, 79.789, {}};
+//CHECK:  VarDecl {{.*}} Advanced_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::A' {{[{][{]}}1, 7.00e-01}, 1, 7.978900e+01, {9}}
+
+}
+
+namespace Vector {
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Vector_Int = (v4si){8, 2, 3};
+//CHECK:  VarDecl {{.*}} Vector_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'Vector::v4si':'__attribute__((__vector_size__(4 * sizeof(int int' {8, 2, 3, 0}
+
+}
+
+namespace Array {
+
+constexpr int Array_Int[] = {1, 2, 3, 4, 5, 6};
+//CHECK:  VarDecl {{.*}} Array_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int [6]' {1, 2, 3, 4, 5, 6}
+
+struct A {
+  int i = 789;
+  double d = 67890.09876;
+};
+
+constexpr A Array2_Struct[][3] = {{{}, {-45678, 9.8}, {9}}, {{}}};
+//CHECK:  VarDecl {{.*}} Array2_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'Array::A const [2][3]' {{[{][{]}}{789, 6.789010e+04}, {-45678, 9.80e+00}, {9, 6.789010e+04}}, {{[{][{]}}789, 6.789010e+04}, {789, 6.789010e+04}, {789, 6.789010e+04}}}
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Array_Vector[] = {{1, 2, 3, 4}, {4, 5, 6, 7}};
+//CHECK:  VarDecl {{.*}} Array_Vector
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Array::v4si [2]' {{[{][{]}}1, 2, 3, 4}, {4, 5, 6, 7}}
+
+}
+
+namespace Union {
+
+struct A {
+  int i = 6789;
+  float f = 987.9876;
+};
+
+union U {
+  int i;
+  A a{567890, 9876.5678f};
+};
+
+constexpr U Unique_Union1 = U{0};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.i = 0}
+
+constexpr U Unique_Union2 = U{};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.a = {567890, 9.876567e+03}}
+
+}
+
+namespace MemberPointer{
+
+struct A {
+  struct B {
+struct C {
+  struct D {
+struct E {
+  struct F {
+struct G {
+  int i;
+};
+  };
+};
+  };
+};
+  };
+};
+
+constexpr auto MemberPointer1 = ::B::C::D::E::F::G::i;
+//CHECK:  VarDecl {{.*}} MemberPointer1
+//CHECK-NEXT: ConstantExpr {{.*}} 'int MemberPointer::A::B::C::D::E::F::G::*' ::i
+
+struct A1 {
+  struct B1 {
+int f() const {
+  return 0;
+}
+  };
+
+};
+
+constexpr auto MemberPointer2 = ::B1::f;
+//CHECK:  VarDecl {{.*}} MemberPointer2
+//CHECK-NEXT: ConstantExpr {{.*}} 'int (MemberPointer::A1::B1::*)() const' ::f
+
+}
+
+namespace std {
+  struct type_info;
+};
+
+namespace LValue {
+
+constexpr int LValueInt = 0;
+constexpr const int& ConstIntRef = LValueInt;
+//CHECK:  VarDecl {{.*}} ConstIntRef
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int' lvalue 

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: clang/include/clang/AST/APValue.h:618
   }
+  const CXXRecordDecl **getMemberPointerPathPtr();
 };

Tyker wrote:
> aaron.ballman wrote:
> > We're horribly inconsistent in this class, but because the other private 
> > member functions go with it, this should probably be 
> > `GetMemberPointerPathPtr()`. Maybe rename the get/setLValue methods from 
> > above as well?
> > We're horribly inconsistent in this class
> 
> this class has many flaws. but is far too broadly used to fix.
Agreed -- I wasn't suggesting to fix the whole class, but just the new APIs 
that we add to the class. It looks like the private functions most consistently 
use a capital letter in this class, unfortunately. Best to stick with the local 
convention when in conflict.



Comment at: clang/include/clang/AST/ASTContext.h:275
-  /// Used to cleanups APValues stored in the AST.
-  mutable llvm::SmallVector APValueCleanups;
-

Tyker wrote:
> aaron.ballman wrote:
> > Tyker wrote:
> > > aaron.ballman wrote:
> > > > Why are you getting rid of this? It seems like we would still want 
> > > > these cleaned up.
> > > when i added APValueCleanups i wasn't aware that there were a generic 
> > > system to handle this. but with this patch APValue a cleaned up using the 
> > > generic ASTContext::addDestruction.
> > I don't see any new calls to `addDestruction()` though. Have I missed 
> > something?
> the modification to use `addDestruction()` was made in a previous revision 
> (https://reviews.llvm.org/D63376).
> the use is currently on master in `ConstantExpr::MoveIntoResult` in the 
> RSK_APValue case of the switch.
> this is just a removing an unused member.
> 
Ahhh, thank you for the explanation, I was missing that context.



Comment at: clang/include/clang/AST/PrettyPrinter.h:203
 
+  /// Wether null pointers should be printed as nullptr or as NULL.
+  unsigned UseNullptr : 1;

Wether -> Whether



Comment at: clang/lib/AST/APValue.cpp:748
 
+APValue::LValuePathEntry *APValue::getLValuePathPtr() {
+  return ((LV *)(char *)Data.buffer)->getPath();

Tyker wrote:
> aaron.ballman wrote:
> > Tyker wrote:
> > > aaron.ballman wrote:
> > > > Can this function be marked `const`?
> > > this function gives access to non-const internal data. this function is 
> > > private so the impact is quite limited.
> > That makes it harder to call this helper from a constant context. I think 
> > there should be overloads (one `const`, one not) to handle this.
> this helper is not intended to be used outside of importing and 
> serialization. it is logically part of initialization.
> normal users are intended to use `ArrayRef 
> APValue::getLValuePath() const`
Nothing about this API suggests that. The name looks like a generic getter. 
Perhaps a more descriptive name and some comments would help?



Comment at: clang/lib/AST/APValue.cpp:537
 
-  if (const ValueDecl *VD = Base.dyn_cast())
+  if (const ValueDecl *VD = Base.dyn_cast())
 Out << *VD;

Since you're touching the code anyway, this can be `const auto *`.



Comment at: clang/lib/AST/APValue.cpp:599
 Out << '[' << Path[I].getAsArrayIndex() << ']';
-ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+ElemTy = cast(ElemTy)->getElementType();
   }

Are you sure this doesn't change behavior? See the implementation of 
`ASTContext::getAsArrayType()`. Same question applies below.



Comment at: clang/lib/AST/APValue.cpp:614
   case APValue::Array: {
-const ArrayType *AT = Ctx.getAsArrayType(Ty);
+const ArrayType *AT = cast(Ty);
 QualType ElemTy = AT->getElementType();

`const auto *` and same question about behavior changes.



Comment at: clang/test/ASTMerge/APValue/APValue.cpp:1
+
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch

Tyker wrote:
> aaron.ballman wrote:
> > Can remove the spurious newline. Also, it seems this file was added with 
> > svn properties, was that intentional (we don't usually do that, FWIW)?
> it wasn't intentional, i added via `git add` i don't think i did anything 
> weird. is it a problem ?
No idea; I'm on a platform where file modes are ignored. You should probably 
drop the svn property.



Comment at: clang/test/ASTMerge/APValue/APValue.cpp:2-3
+
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -ast-merge %t.pch -ast-dump-all | 
FileCheck %s
+

Tyker wrote:
> aaron.ballman wrote:
> > no need for `-x c++` is there? This is already a C++ compilation unit.
> i don't know if it is normal. but i am getting an error hen i am not using 
> `-x c++`
> `error: invalid argument 

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 221384.
Tyker marked 12 inline comments as done.
Tyker retitled this revision from "[clang] Improve Serialization/Imporing of 
APValues" to "[clang] Improve Serialization/Imporing/Dumping of APValues".
Tyker edited the summary of this revision.
Tyker added a comment.

fixed some changes.
see comments for others.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640

Files:
  clang/include/clang/AST/APValue.h
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTImporter.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/PrettyPrinter.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/AST/ast-dump-color.cpp
  clang/test/ASTMerge/APValue/APValue.cpp

Index: clang/test/ASTMerge/APValue/APValue.cpp
===
--- /dev/null
+++ clang/test/ASTMerge/APValue/APValue.cpp
@@ -0,0 +1,209 @@
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -ast-merge %t.pch -ast-dump-all | FileCheck %s
+
+#ifndef EMIT
+#define EMIT
+
+namespace Integer {
+
+constexpr int Unique_Int = int(6789);
+//CHECK:  VarDecl {{.*}} Unique_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'int' 6789
+
+constexpr __uint128_t Unique_Int128 = ((__uint128_t)0x75f17d6b3588f843 << 64) | 0xb13dea7c9c324e51;
+//CHECK:  VarDecl {{.*}} Unique_Int128 
+//CHECK-NEXT: ConstantExpr {{.*}} 'unsigned __int128' 156773562844924187900898496343692168785
+
+}
+
+namespace FloatingPoint {
+
+constexpr double Unique_Double = double(567890.67890);
+//CHECK:  VarDecl {{.*}} Unique_Double
+//CHECK-NEXT: ConstantExpr {{.*}} 'double' 5.678907e+05
+
+}
+
+// FIXME: Add test for FixedPoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
+namespace Struct {
+
+struct B {
+  int i;
+  double d;
+};
+
+constexpr B Basic_Struct = B{1, 0.7};
+//CHECK:  VarDecl {{.*}} Basic_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::B' {1, 7.00e-01}
+
+struct C {
+  int i = 9;
+};
+
+struct A : B {
+  int i;
+  double d;
+  C c;
+};
+
+constexpr A Advanced_Struct = A{Basic_Struct, 1, 79.789, {}};
+//CHECK:  VarDecl {{.*}} Advanced_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::A' {{[{][{]}}1, 7.00e-01}, 1, 7.978900e+01, {9}}
+
+}
+
+namespace Vector {
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Vector_Int = (v4si){8, 2, 3};
+//CHECK:  VarDecl {{.*}} Vector_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'Vector::v4si':'__attribute__((__vector_size__(4 * sizeof(int int' {8, 2, 3, 0}
+
+}
+
+namespace Array {
+
+constexpr int Array_Int[] = {1, 2, 3, 4, 5, 6};
+//CHECK:  VarDecl {{.*}} Array_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int [6]' {1, 2, 3, 4, 5, 6}
+
+struct A {
+  int i = 789;
+  double d = 67890.09876;
+};
+
+constexpr A Array2_Struct[][3] = {{{}, {-45678, 9.8}, {9}}, {{}}};
+//CHECK:  VarDecl {{.*}} Array2_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'Array::A const [2][3]' {{[{][{]}}{789, 6.789010e+04}, {-45678, 9.80e+00}, {9, 6.789010e+04}}, {{[{][{]}}789, 6.789010e+04}, {789, 6.789010e+04}, {789, 6.789010e+04}}}
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Array_Vector[] = {{1, 2, 3, 4}, {4, 5, 6, 7}};
+//CHECK:  VarDecl {{.*}} Array_Vector
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Array::v4si [2]' {{[{][{]}}1, 2, 3, 4}, {4, 5, 6, 7}}
+
+}
+
+namespace Union {
+
+struct A {
+  int i = 6789;
+  float f = 987.9876;
+};
+
+union U {
+  int i;
+  A a{567890, 9876.5678f};
+};
+
+constexpr U Unique_Union1 = U{0};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.i = 0}
+
+constexpr U Unique_Union2 = U{};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.a = {567890, 9.876567e+03}}
+
+}
+
+namespace MemberPointer{
+
+struct A {
+  struct B {
+struct C {
+  struct D {
+struct E {
+  struct F {
+struct G {
+  int i;
+};
+  };
+};
+  };
+};
+  };
+};
+
+constexpr auto MemberPointer1 = ::B::C::D::E::F::G::i;
+//CHECK:  VarDecl {{.*}} MemberPointer1
+//CHECK-NEXT: ConstantExpr {{.*}} 'int MemberPointer::A::B::C::D::E::F::G::*' ::i
+
+struct A1 {
+  struct B1 {
+int f() const {
+  return 0;
+}
+  };
+
+};
+
+constexpr auto MemberPointer2 = ::B1::f;
+//CHECK:  VarDecl {{.*}} MemberPointer2
+//CHECK-NEXT: ConstantExpr {{.*}} 'int (MemberPointer::A1::B1::*)() const' ::f
+
+}
+
+namespace std {
+  

[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Tyker via Phabricator via cfe-commits]

Tyker added inline comments.



Comment at: clang/include/clang/AST/APValue.h:618
   }
+  const CXXRecordDecl **getMemberPointerPathPtr();
 };

aaron.ballman wrote:
> We're horribly inconsistent in this class, but because the other private 
> member functions go with it, this should probably be 
> `GetMemberPointerPathPtr()`. Maybe rename the get/setLValue methods from 
> above as well?
> We're horribly inconsistent in this class

this class has many flaws. but is far too broadly used to fix.



Comment at: clang/include/clang/AST/APValue.h:512
   }
-  void setVector(const APValue *E, unsigned N) {
+  void ReserveVector(unsigned N) {
 assert(isVector() && "Invalid accessor");

aaron.ballman wrote:
> aaron.ballman wrote:
> > `reserveVector` per naming conventions
> This was marked as done but is still an issue.
sorry



Comment at: clang/include/clang/AST/ASTContext.h:275
-  /// Used to cleanups APValues stored in the AST.
-  mutable llvm::SmallVector APValueCleanups;
-

aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > Why are you getting rid of this? It seems like we would still want these 
> > > cleaned up.
> > when i added APValueCleanups i wasn't aware that there were a generic 
> > system to handle this. but with this patch APValue a cleaned up using the 
> > generic ASTContext::addDestruction.
> I don't see any new calls to `addDestruction()` though. Have I missed 
> something?
the modification to use `addDestruction()` was made in a previous revision 
(https://reviews.llvm.org/D63376).
the use is currently on master in `ConstantExpr::MoveIntoResult` in the 
RSK_APValue case of the switch.
this is just a removing an unused member.




Comment at: clang/include/clang/AST/TextNodeDumper.h:149
 
-  const ASTContext *Context;
+  const ASTContext *Context = nullptr;
 

aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > Good catch -- this pointed out a bug in the class that I've fixed in 
> > > r372323, so you'll need to rebase.
> > i took a look at the revision. there is a big difference is the quality of 
> > output between APValue::dump and APValue::printPretty. i think it is 
> > possible to come quite close to printPretty's output even without the 
> > ASTContext. this would require having a default PrintingPolicy and 
> > improving dump
> > 
> > in this patch i was relying on the -ast-dump output for testing. i would 
> > need to find an other testing strategy or make the improvement to 
> > APValue::dump first.
> > there is a big difference is the quality of output between APValue::dump 
> > and APValue::printPretty.
> 
> Yes, there is.
> 
> > i think it is possible to come quite close to printPretty's output even 
> > without the ASTContext. this would require having a default PrintingPolicy 
> > and improving dump
> 
> That would be much-appreciated! When I looked at it, it seemed like it may 
> not be plausible because `Stmt` does not track which `ASTContext` it is 
> associated with the same way that `Decl` does, and changing that seemed 
> likely to cause a huge amount of interface churn.
> 
> > in this patch i was relying on the -ast-dump output for testing. i would 
> > need to find an other testing strategy or make the improvement to 
> > APValue::dump first.
> 
> The issue resolved by r372323 was that we would crash on certain kinds of AST 
> dumps. Specifically, the default AST dumper is often used during a debugging 
> session to dump AST node information within the debugger. It was trivial to 
> get that to crash before r372323, but with that revision, we no longer crash 
> but get slightly uglier output (which is acceptable because it's still 
> human-readable output).
> 
> I'm sorry for causing extra pain for you here, but I didn't want the fix from 
> this review to accidentally become an enshrined part of the API because it's 
> very easy to forget about this use case when working on AST dumping 
> functionality.
no worries, i wrote the original bug. i added APValue::dumpPretty which has 
almost the same output as APValue::printPretty but doesn't need an ASTContext. 
and is used for TextNodeDumper.



Comment at: clang/lib/AST/APValue.cpp:748
 
+APValue::LValuePathEntry *APValue::getLValuePathPtr() {
+  return ((LV *)(char *)Data.buffer)->getPath();

aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > Can this function be marked `const`?
> > this function gives access to non-const internal data. this function is 
> > private so the impact is quite limited.
> That makes it harder to call this helper from a constant context. I think 
> there should be overloads (one `const`, one not) to handle this.
this helper is not intended to be used outside of importing and serialization. 
it is logically part of initialization.
normal users are intended to use `ArrayRef