[PATCH] D158502: [clang][Interp] Actually consider ConstantExpr result

[Timm Bäder via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG293ae0b4b45f: [clang][Interp] Actually consider ConstantExpr 
result (authored by tbaeder).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158502

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp


Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -634,8 +634,14 @@
 
 template 
 bool ByteCodeExprGen::VisitConstantExpr(const ConstantExpr *E) {
-  // TODO: Check if the ConstantExpr already has a value set and if so,
-  //   use that instead of evaluating it again.
+  // Try to emit the APValue directly, without visiting the subexpr.
+  // This will only fail if we can't emit the APValue, so won't emit any
+  // diagnostics or any double values.
+  std::optional T = classify(E->getType());
+  if (T && E->hasAPValueResult() &&
+  this->visitAPValue(E->getAPValueResult(), *T, E))
+return true;
+
   return this->delegate(E->getSubExpr());
 }
 


Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -634,8 +634,14 @@
 
 template 
 bool ByteCodeExprGen::VisitConstantExpr(const ConstantExpr *E) {
-  // TODO: Check if the ConstantExpr already has a value set and if so,
-  //   use that instead of evaluating it again.
+  // Try to emit the APValue directly, without visiting the subexpr.
+  // This will only fail if we can't emit the APValue, so won't emit any
+  // diagnostics or any double values.
+  std::optional T = classify(E->getType());
+  if (T && E->hasAPValueResult() &&
+  this->visitAPValue(E->getAPValueResult(), *T, E))
+return true;
+
   return this->delegate(E->getSubExpr());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158502: [clang][Interp] Actually consider ConstantExpr result

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D158502

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

[PATCH] D158502: [clang][Interp] Actually consider ConstantExpr result

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 556474.
tbaeder marked 2 inline comments as done.

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

https://reviews.llvm.org/D158502

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp


Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -733,8 +733,14 @@
 
 template 
 bool ByteCodeExprGen::VisitConstantExpr(const ConstantExpr *E) {
-  // TODO: Check if the ConstantExpr already has a value set and if so,
-  //   use that instead of evaluating it again.
+  // Try to emit the APValue directly, without visiting the subexpr.
+  // This will only fail if we can't emit the APValue, so won't emit any
+  // diagnostics or any double values.
+  std::optional T = classify(E->getType());
+  if (T && E->hasAPValueResult() &&
+  this->visitAPValue(E->getAPValueResult(), *T, E))
+return true;
+
   return this->delegate(E->getSubExpr());
 }
 


Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -733,8 +733,14 @@
 
 template 
 bool ByteCodeExprGen::VisitConstantExpr(const ConstantExpr *E) {
-  // TODO: Check if the ConstantExpr already has a value set and if so,
-  //   use that instead of evaluating it again.
+  // Try to emit the APValue directly, without visiting the subexpr.
+  // This will only fail if we can't emit the APValue, so won't emit any
+  // diagnostics or any double values.
+  std::optional T = classify(E->getType());
+  if (T && E->hasAPValueResult() &&
+  this->visitAPValue(E->getAPValueResult(), *T, E))
+return true;
+
   return this->delegate(E->getSubExpr());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158502: [clang][Interp] Actually consider ConstantExpr result

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

In D158502#4616174 , @cor3ntin wrote:

> FYI there are build failures. you probably need to rebase (visitAPValue does 
> not seem to actually exist)

This depends on the `__builtin_offsetof()` patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158502

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

[PATCH] D158502: [clang][Interp] Actually consider ConstantExpr result

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

FYI there are build failures. you probably need to rebase (visitAPValue does 
not seem to actually exist)




Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:767-772
+  std::optional T = classify(E->getType());
+  if (T && E->hasAPValueResult() &&
+  this->visitAPValue(E->getAPValueResult(), *T, E))
+return true;
+
   return this->delegate(E->getSubExpr());

tbaeder wrote:
> cor3ntin wrote:
> > so if `visitAPValue` fails, we continue. Couldn't that lead to duplicated 
> > diagnostics? Shouldn't we simply return whatever `visitAPValue` returns 
> > unconditionally?
> I did it this way because we don't handle all types of APValues in 
> `visitAPValue()` (think lvalues with an lvalue path), so in those cases we 
> need to visit the expression instead. If there is an `APValue` we're visting, 
> we're not emitting any diagnostics I think.
Can you add a comment explaining that? I think it would help! thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158502

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

[PATCH] D158502: [clang][Interp] Actually consider ConstantExpr result

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:767-772
+  std::optional T = classify(E->getType());
+  if (T && E->hasAPValueResult() &&
+  this->visitAPValue(E->getAPValueResult(), *T, E))
+return true;
+
   return this->delegate(E->getSubExpr());

cor3ntin wrote:
> so if `visitAPValue` fails, we continue. Couldn't that lead to duplicated 
> diagnostics? Shouldn't we simply return whatever `visitAPValue` returns 
> unconditionally?
I did it this way because we don't handle all types of APValues in 
`visitAPValue()` (think lvalues with an lvalue path), so in those cases we need 
to visit the expression instead. If there is an `APValue` we're visting, we're 
not emitting any diagnostics I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158502

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

[PATCH] D158502: [clang][Interp] Actually consider ConstantExpr result

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:767-772
+  std::optional T = classify(E->getType());
+  if (T && E->hasAPValueResult() &&
+  this->visitAPValue(E->getAPValueResult(), *T, E))
+return true;
+
   return this->delegate(E->getSubExpr());

so if `visitAPValue` fails, we continue. Couldn't that lead to duplicated 
diagnostics? Shouldn't we simply return whatever `visitAPValue` returns 
unconditionally?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158502

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

[PATCH] D158502: [clang][Interp] Actually consider ConstantExpr result

[Timm Bäder via Phabricator via cfe-commits]

tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, erichkeane, shafik, cor3ntin.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

  Since we have visitAPValue now, we might as well use it here.

Not really possible to test this, so no test included.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158502

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp


Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -764,8 +764,11 @@
 
 template 
 bool ByteCodeExprGen::VisitConstantExpr(const ConstantExpr *E) {
-  // TODO: Check if the ConstantExpr already has a value set and if so,
-  //   use that instead of evaluating it again.
+  std::optional T = classify(E->getType());
+  if (T && E->hasAPValueResult() &&
+  this->visitAPValue(E->getAPValueResult(), *T, E))
+return true;
+
   return this->delegate(E->getSubExpr());
 }
 


Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -764,8 +764,11 @@
 
 template 
 bool ByteCodeExprGen::VisitConstantExpr(const ConstantExpr *E) {
-  // TODO: Check if the ConstantExpr already has a value set and if so,
-  //   use that instead of evaluating it again.
+  std::optional T = classify(E->getType());
+  if (T && E->hasAPValueResult() &&
+  this->visitAPValue(E->getAPValueResult(), *T, E))
+return true;
+
   return this->delegate(E->getSubExpr());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits