[PATCH] D135569: [clang][Interp] Don't run functions immediately after compiling them

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1928da1ef73c: [clang][Interp] Dont run functions 
immediately after compiling them (authored by tbaeder).

Changed prior to commit:
  https://reviews.llvm.org/D135569?vs=466462=467706#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135569

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


Index: clang/lib/AST/Interp/Context.cpp
===
--- clang/lib/AST/Interp/Context.cpp
+++ clang/lib/AST/Interp/Context.cpp
@@ -39,11 +39,7 @@
 }
   }
 
-  if (!Func->isConstexpr())
-return false;
-
-  APValue Dummy;
-  return Run(Parent, Func, Dummy);
+  return Func->isConstexpr();
 }
 
 bool Context::evaluateAsRValue(State , const Expr *E, APValue ) {


Index: clang/lib/AST/Interp/Context.cpp
===
--- clang/lib/AST/Interp/Context.cpp
+++ clang/lib/AST/Interp/Context.cpp
@@ -39,11 +39,7 @@
 }
   }
 
-  if (!Func->isConstexpr())
-return false;
-
-  APValue Dummy;
-  return Run(Parent, Func, Dummy);
+  return Func->isConstexpr();
 }
 
 bool Context::evaluateAsRValue(State , const Expr *E, APValue ) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D135569: [clang][Interp] Don't run functions immediately after compiling them

[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.

I think this change makes sense to me. Ultimately, this API is about the 
potential to be constexpr, so there's no need to pay for the expense of running 
the function with some ginned up arguments -- if we can compile it as a 
constexpr function, it's potentially constexpr.




Comment at: clang/lib/AST/Interp/Context.cpp:44-47
   if (!Func->isConstexpr())
 return false;
 
+  return true;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135569

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

[PATCH] D135569: [clang][Interp] Don't run functions immediately after compiling them

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

tbaeder added a comment.

In D135569#3847493 , @aaron.ballman 
wrote:

> This function is testing whether something is potentially a constant 
> expression. Something might not be a valid constant expression for two 
> reasons: 1) it uses some prohibited language construct, 2) it hit undefined 
> behavior. You don't know if you hit undefined behavior until you run the 
> function, so could that be why the function was being run? However, I don't 
> know how you would run an arbitrary function that might accept arguments, so 
> the `Run` call does look suspicious -- especially because it landed in the 
> initial patch of the functionality (https://reviews.llvm.org/D64146) without 
> comment.

Yes. A few weeks ago I thought it might make sense to actually set functions to 
invalid if `isPotentialConstantExpr()` returns `false`, but found out that it 
returns `false` all the time, e.g. if the function expects parameters.

I think the whole "lets compile all constexpr functions" approach will probably 
go away at some point anyway? When including stdlib headers, we're probably 
compiling a lot of code that will never run...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135569

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

[PATCH] D135569: [clang][Interp] Don't run functions immediately after compiling them

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This function is testing whether something is potentially a constant 
expression. Something might not be a valid constant expression for two reasons: 
1) it uses some prohibited language construct, 2) it hit undefined behavior. 
You don't know if you hit undefined behavior until you run the function, so 
could that be why the function was being run? However, I don't know how you 
would run an arbitrary function that might accept arguments, so the `Run` call 
does look suspicious -- especially because it landed in the initial patch of 
the functionality (https://reviews.llvm.org/D64146) without comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135569

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

[PATCH] D135569: [clang][Interp] Don't run functions immediately after compiling them

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

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

I'm not 100% sure why this was done in the first place, but it doesn't work 
when the function makes assumptions about the stack of the caller, e.g. in the 
RVO case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135569

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


Index: clang/lib/AST/Interp/Context.cpp
===
--- clang/lib/AST/Interp/Context.cpp
+++ clang/lib/AST/Interp/Context.cpp
@@ -44,8 +44,7 @@
   if (!Func->isConstexpr())
 return false;
 
-  APValue Dummy;
-  return Run(Parent, Func, Dummy);
+  return true;
 }
 
 bool Context::evaluateAsRValue(State , const Expr *E, APValue ) {


Index: clang/lib/AST/Interp/Context.cpp
===
--- clang/lib/AST/Interp/Context.cpp
+++ clang/lib/AST/Interp/Context.cpp
@@ -44,8 +44,7 @@
   if (!Func->isConstexpr())
 return false;
 
-  APValue Dummy;
-  return Run(Parent, Func, Dummy);
+  return true;
 }
 
 bool Context::evaluateAsRValue(State , const Expr *E, APValue ) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits