[PATCH] D76443: Use ConstantExpr cached APValues if present for code generation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders abandoned this revision.
wchilders added a comment.

Abandoned in favor of https://reviews.llvm.org/D76420


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

https://reviews.llvm.org/D76443



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

[PATCH] D85933: Don't track consteval references for dependent members

[Wyatt Childers via Phabricator via cfe-commits]

wchilders created this revision.
wchilders added reviewers: Tyker, rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
wchilders requested review of this revision.

Previously dependent references to consteval (`ReferenceToConsteval`) were 
tracked, and dependent expressions were evaluated as possible immediate 
invocations.

This resulted in the evaluation of value dependent expressions.

This patch also suppresses duplicated diagnostics in debug builds while working 
with templates caused by the side effects of `FixOverloadedFunctionReference`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85933

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -104,6 +104,87 @@
 
 }
 
+namespace dependent_addressing {
+
+template
+consteval int t_f_eval(int i) {
+// expected-note@-1+ {{declared here}}
+  return i;
+}
+
+using func_type = int(int);
+
+template
+auto t_f_eval_proxy() {
+  return &t_f_eval;
+// expected-error@-1 {{take address}}
+}
+
+template
+struct A {
+  consteval int f1(int i) const {
+// expected-note@-1 {{declared here}}
+return i + y;
+  }
+  consteval A(int i);
+  consteval A() = default;
+  consteval ~A() = default; // expected-error {{destructor cannot be declared consteval}}
+};
+
+template
+constexpr auto l_eval = [](int i) consteval {
+// expected-note@-1+ {{declared here}}
+
+  return i + y;
+};
+
+template
+int test_templ() {
+  func_type* p1 = (&t_f_eval);
+  // expected-error@-1+ {{cannot take address of consteval function 't_f_eval<1>' outside of an immediate invocation}}
+
+  auto ptr = &t_f_eval;
+  // expected-error@-1 {{take address}}
+
+  // FIXME: AddressOfFunctionResolver breaks
+  // func_type* p7 = __builtin_addressof(t_f_eval);
+
+  auto p = t_f_eval;
+  // expected-error@-1 {{take address}}
+
+  auto m1 = &A::f1;
+  // expected-error@-1 {{take address}}
+  auto l1 = &decltype(l_eval)::operator();
+  // expected-error@-1 {{take address}}
+
+  auto pr = t_f_eval_proxy();
+  // expected-note@-1 {{in instantiation of function template specialization}}
+
+  return 0;
+}
+
+auto tr = test_templ<1>();
+// expected-note@-1+ {{in instantiation of function template specialization}}
+
+} // namespace dependent_addressing
+
+namespace dependent_call {
+
+consteval int cf1(int y) {
+  return y;
+}
+
+template
+auto f1() {
+  constexpr int x = cf1(y);
+  return x;
+}
+
+auto f1r = f1<1>();
+
+} // namespace dependent_call
+
+
 namespace invalid_function {
 
 struct A {
@@ -593,4 +674,5 @@
   { Copy* c; c = new Copy(to_lvalue_ref(std::move(Copy(&f_eval; }// expected-error {{is not a constant expression}} expected-note {{to a consteval}}
 }
 
-} // namespace special_ctor
+} // namespace copy_ctor
+
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -34,6 +34,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SaveAndRestore.h"
 #include 
 #include 
 
@@ -1750,11 +1751,19 @@
   }
 
   // Check that we've computed the proper type after overload resolution.
+#ifndef NDEBUG
   // FIXME: FixOverloadedFunctionReference has side-effects; we shouldn't
   // be calling it from within an NDEBUG block.
+
+  // This is a hack to prevent duplicated diagnostics triggered by
+  // the above side effects in some cases involving immediate invocations.
+  llvm::SaveAndRestore DisableIITracking(
+  S.RebuildingImmediateInvocation, true);
+
   assert(S.Context.hasSameType(
 FromType,
 S.FixOverloadedFunctionReference(From, AccessPair, Fn)->getType()));
+#endif
 } else {
   return false;
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16146,11 +16146,13 @@
 }
 
 ExprResult Sema::CheckForImmediateInvocation(ExprResult E, FunctionDecl *Decl) {
+  // Skip checking dependent expressions - If we allow dependent expressions
+  // we end up evaluating dependent constant expressions.
   if (!E.isUsable() || !Decl || !Decl->isConsteval() || isConstantEvaluated() ||
-  RebuildingImmediateInvocation)
+  E.get()->getDependence() || RebuildingImmediateInvocation)
 return E;
 
-  /// Opportunistically remove the callee from ReferencesToConsteval if we can.
+  /// Opportunistically remove the callee from ReferenceToConsteval if we can.
   /// It's OK if this fails; we'll also remove this in
   /// HandleImmediateInvocations, but catching it here allows us to avoid
   /// walking the AST looking for it 

[PATCH] D85933: Don't track consteval references for dependent members

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:1761
+  llvm::SaveAndRestore DisableIITracking(
+  S.RebuildingImmediateInvocation, true);
+

Tyker wrote:
> I don't think RebuildingImmediateInvocation should be used here since we are 
> not rebuilding immediate invocations
> 
> setSuppressAllDiagnostics or TentativeAnalysisScope seems more adapted.
These don't actually work for this use case due to the queuing nature of the 
diagnostic. Given the hacky nature of this assertion as it stands, I didn't 
want to further pollute the code base.

I'm open to suggestions. If you're proposing making `MarkDeclRefReferenced` not 
queue in response to diagnostic suppression, that doesn't seem totally 
unreasonable; we'll just have to add the additional condition to check the 
diagnostic state in `MarkDeclRefReferenced`.



Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:131
+  consteval A() = default;
+  consteval ~A() = default; // expected-error {{destructor cannot be declared 
consteval}}
+};

Tyker wrote:
> could you check that this diagnostic and those like it doesn't appear when 
> the struct A isn't instantiated.
I duplicated this namespace without any of the instantiation, and it does seem 
to trigger the diagnostic "destructor cannot be declared consteval" -- all 
other diagnostics are silent. Is that particularly undesirable? 

Also, WRT testing. would that the best option here (having a duplicated 
namespace, "dependent_addressing_uninstantiated" -- without the line triggering 
instantiation)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85933

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

[PATCH] D92733: Fix PR25627 - false positive diagnostics involving implicit-captures in dependent lambda expressions.

[Wyatt Childers via Phabricator via cfe-commits]

wchilders requested changes to this revision.
wchilders added a comment.
This revision now requires changes to proceed.

I'm still "chewing on this", I'm not sure I fully understand the problem well 
enough to give great "big picture" feedback. In any case, I've left a few 
comments where I had a potentially helpful thought :)




Comment at: clang/lib/Sema/SemaExprCXX.cpp:7743
+  const VarDecl *StaticDataMember = dyn_cast(E->getMemberDecl());
+  if (StaticDataMember) {
+Results.push_back(E);

This might be better restated as:

```
if (isa(E->getMemberDecl())) {
```



Comment at: clang/lib/Sema/SemaExprCXX.cpp:7754
+  Visit(E->getBase());
+  VisitingArraySubscriptBaseExpr = false;
+}

This likely should restore the previous state to prevent preemptive clearing of 
the flag, i.e.
```
bool WasVisitingArraySubscriptBase = VisitingArraySubscriptBaseExpr;
VisitingArraySubscriptBaseExpr = true;
Visit(E->getBase());
VisitingArraySubscriptBaseExpr = WasVisitingArraySubscriptBase;
```

I haven't given sufficient thought as to whether that would be problematic in 
this case; but it seems worth bringing up.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:7813
+  CheckLValueToRValueConversionOperand(E, /*DiscardedValue=*/true);
+  E = ER.get();
 }

This should probably handle the error case, no?



Comment at: clang/lib/Sema/SemaExprCXX.cpp:7881
+  if (Var->getType()->isDependentType())
+return false;
 

Could you elaborate on the changes made to this function -- if for no other 
reason than my curiosity :)? This seems to be largely the same checks, in a 
different order and some control flow paths that used to return false now 
potentially return true and vice versa.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:8016
+IsNonCaptureableLambda && IsInitializingAnEntityWithDependentType &&
+!IsReferenceRelatedAutoTypeDeduction) {
+

Is there a succinct way to rewrite this that might improve readability? i.e. 

```
bool SuccinctName = !IsFullExprInstantiationDependent && 
!IsVarNeverAConstantExpression &&
IsNonCaptureableLambda && IsInitializingAnEntityWithDependentType &&
!IsReferenceRelatedAutoTypeDeduction;
if (SuccinctName) {
```

Alternatively a short comment; It's pretty hard (at least for me) to tell at a 
glance what this branch is for.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:8037
+//   2) the VarDecl can never be a constant expression. 
+//   3) we  are initializing to a reference-related type (via auto)
+//   4) we are  initializing to the declaration type of the VarDecl, and

Minor, but there's an extra space here the pre-merge didn't catch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92733

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

[PATCH] D90976: Fixed an issue where diagnostics printed expressions in a roundabout way

[Wyatt Childers via Phabricator via cfe-commits]

wchilders created this revision.
wchilders added reviewers: yaxunl, hans, rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
wchilders requested review of this revision.

Previously expressions were being printed in diagnostics via an implicit 
conversion to an expression template argument -- which would then be printed 
via some code with a comment suggesting the code path was only for completeness 
and rarely/never used; this was both roundabout, and subtle. This patch 
provides a more direct overload, though the patch is still imperfect, as it 
does not solve the associated LangOpts issue.

Additionally, to prevent similar issues in the future, this patch makes 
conversions from expressions to template arguments explicit.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90976

Files:
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/TemplateBase.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/SemaTemplateVariadic.cpp
  clang/lib/Sema/TreeTransform.h

Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -3542,7 +3542,7 @@
   if (Result.isInvalid())
 return TemplateArgumentLoc();
 
-  return TemplateArgumentLoc(Result.get(), Result.get());
+  return TemplateArgumentLoc(TemplateArgument(Result.get()), Result.get());
 }
 
 case TemplateArgument::Template:
@@ -3822,7 +3822,8 @@
   Expr *Pattern = Expansion->getPattern();
 
   SmallVector Unexpanded;
-  getSema().collectUnexpandedParameterPacks(Pattern, Unexpanded);
+  getSema().collectUnexpandedParameterPacks(
+  TemplateArgument(Pattern), Unexpanded);
   assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
 
   // Determine whether the set of unexpanded parameter packs can and should
@@ -12420,7 +12421,8 @@
  ->getTypeLoc()
  .castAs();
   SmallVector Unexpanded;
-  SemaRef.collectUnexpandedParameterPacks(OldVD->getInit(), Unexpanded);
+  SemaRef.collectUnexpandedParameterPacks(
+  TemplateArgument(OldVD->getInit()), Unexpanded);
 
   // Determine whether the set of unexpanded parameter packs can and should
   // be expanded.
@@ -13016,8 +13018,8 @@
 E->getPackLoc());
 if (DRE.isInvalid())
   return ExprError();
-ArgStorage = new (getSema().Context) PackExpansionExpr(
-getSema().Context.DependentTy, DRE.get(), E->getPackLoc(), None);
+ArgStorage = TemplateArgument(new (getSema().Context) PackExpansionExpr(
+getSema().Context.DependentTy, DRE.get(), E->getPackLoc(), None));
   }
   PackArgs = ArgStorage;
 }
@@ -13155,7 +13157,8 @@
   Expr *Pattern = E->getPattern();
 
   SmallVector Unexpanded;
-  getSema().collectUnexpandedParameterPacks(Pattern, Unexpanded);
+  getSema().collectUnexpandedParameterPacks(
+  TemplateArgument(Pattern), Unexpanded);
   assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
 
   // Determine whether the set of unexpanded parameter packs can and should
@@ -13351,8 +13354,10 @@
 if (OrigElement.isPackExpansion()) {
   // This key/value element is a pack expansion.
   SmallVector Unexpanded;
-  getSema().collectUnexpandedParameterPacks(OrigElement.Key, Unexpanded);
-  getSema().collectUnexpandedParameterPacks(OrigElement.Value, Unexpanded);
+  getSema().collectUnexpandedParameterPacks(
+  TemplateArgument(OrigElement.Key), Unexpanded);
+  getSema().collectUnexpandedParameterPacks(
+  TemplateArgument(OrigElement.Value), Unexpanded);
   assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
 
   // Determine whether the set of unexpanded parameter packs can
Index: clang/lib/Sema/SemaTemplateVariadic.cpp
===
--- clang/lib/Sema/SemaTemplateVariadic.cpp
+++ clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -1089,7 +1089,7 @@
 Expr *Pattern = Expansion->getPattern();
 Ellipsis = Expansion->getEllipsisLoc();
 NumExpansions = Expansion->getNumExpansions();
-return TemplateArgumentLoc(Pattern, Pattern);
+return TemplateArgumentLoc(TemplateArgument(Pattern), Pattern);
   }
 
   case TemplateArgument::TemplateExpansion:
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -109,8 +109,8 @@
 
   SmallVector Unexpanded;
   if

[PATCH] D91011: [NFC, Refactor] Rename the (scoped) enum DeclaratorContext's enumerators to avoid redundancy

[Wyatt Childers via Phabricator via cfe-commits]

wchilders accepted this revision.
wchilders added a comment.

+1 Nice QOL change :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91011

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

[PATCH] D91035: [NFC, Refactor] Convert FunctionDefinitionKind from DeclSpech.h to a scoped enum

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added a comment.

Generally agree with this direction; Are there plans for migrating 
https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/Specifiers.h
 in a similar fashion, for consistency?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91035

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

[PATCH] D90976: Fixed an issue where diagnostics printed expressions in a roundabout way

[Wyatt Childers via Phabricator via cfe-commits]

wchilders updated this revision to Diff 303953.
wchilders added a comment.

Formatting fixes


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

https://reviews.llvm.org/D90976

Files:
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/TemplateBase.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/SemaTemplateVariadic.cpp
  clang/lib/Sema/TreeTransform.h

Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -3542,7 +3542,7 @@
   if (Result.isInvalid())
 return TemplateArgumentLoc();
 
-  return TemplateArgumentLoc(Result.get(), Result.get());
+  return TemplateArgumentLoc(TemplateArgument(Result.get()), Result.get());
 }
 
 case TemplateArgument::Template:
@@ -3822,7 +3822,8 @@
   Expr *Pattern = Expansion->getPattern();
 
   SmallVector Unexpanded;
-  getSema().collectUnexpandedParameterPacks(Pattern, Unexpanded);
+  getSema().collectUnexpandedParameterPacks(TemplateArgument(Pattern),
+Unexpanded);
   assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
 
   // Determine whether the set of unexpanded parameter packs can and should
@@ -12420,7 +12421,8 @@
  ->getTypeLoc()
  .castAs();
   SmallVector Unexpanded;
-  SemaRef.collectUnexpandedParameterPacks(OldVD->getInit(), Unexpanded);
+  SemaRef.collectUnexpandedParameterPacks(
+  TemplateArgument(OldVD->getInit()), Unexpanded);
 
   // Determine whether the set of unexpanded parameter packs can and should
   // be expanded.
@@ -13016,8 +13018,8 @@
 E->getPackLoc());
 if (DRE.isInvalid())
   return ExprError();
-ArgStorage = new (getSema().Context) PackExpansionExpr(
-getSema().Context.DependentTy, DRE.get(), E->getPackLoc(), None);
+ArgStorage = TemplateArgument(new (getSema().Context) PackExpansionExpr(
+getSema().Context.DependentTy, DRE.get(), E->getPackLoc(), None));
   }
   PackArgs = ArgStorage;
 }
@@ -13155,7 +13157,8 @@
   Expr *Pattern = E->getPattern();
 
   SmallVector Unexpanded;
-  getSema().collectUnexpandedParameterPacks(Pattern, Unexpanded);
+  getSema().collectUnexpandedParameterPacks(TemplateArgument(Pattern),
+Unexpanded);
   assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
 
   // Determine whether the set of unexpanded parameter packs can and should
@@ -13351,8 +13354,10 @@
 if (OrigElement.isPackExpansion()) {
   // This key/value element is a pack expansion.
   SmallVector Unexpanded;
-  getSema().collectUnexpandedParameterPacks(OrigElement.Key, Unexpanded);
-  getSema().collectUnexpandedParameterPacks(OrigElement.Value, Unexpanded);
+  getSema().collectUnexpandedParameterPacks(
+  TemplateArgument(OrigElement.Key), Unexpanded);
+  getSema().collectUnexpandedParameterPacks(
+  TemplateArgument(OrigElement.Value), Unexpanded);
   assert(!Unexpanded.empty() && "Pack expansion without parameter packs?");
 
   // Determine whether the set of unexpanded parameter packs can
Index: clang/lib/Sema/SemaTemplateVariadic.cpp
===
--- clang/lib/Sema/SemaTemplateVariadic.cpp
+++ clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -1089,7 +1089,7 @@
 Expr *Pattern = Expansion->getPattern();
 Ellipsis = Expansion->getEllipsisLoc();
 NumExpansions = Expansion->getNumExpansions();
-return TemplateArgumentLoc(Pattern, Pattern);
+return TemplateArgumentLoc(TemplateArgument(Pattern), Pattern);
   }
 
   case TemplateArgument::TemplateExpansion:
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -109,8 +109,8 @@
 
   SmallVector Unexpanded;
   if (Aligned->isAlignmentExpr())
-S.collectUnexpandedParameterPacks(Aligned->getAlignmentExpr(),
-  Unexpanded);
+S.collectUnexpandedParameterPacks(
+TemplateArgument(Aligned->getAlignmentExpr()), Unexpanded);
   else
 S.collectUnexpandedParameterPacks(Aligned->getAlignmentType()->getTypeLoc(),
   Unexpanded);
@@ -5402,7 +5402,8 @@
   TypeLoc BaseTL = Init->getTypeSourceInfo()->getTypeLoc();
   SmallVector Unexpanded;
   collectUnexpandedParameterPacks(BaseTL, Une

[PATCH] D91035: [NFC, Refactor] Convert FunctionDefinitionKind from DeclSpech.h to a scoped enum

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added a comment.

Replying to inline comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91035

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

[PATCH] D91035: [NFC, Refactor] Convert FunctionDefinitionKind from DeclSpech.h to a scoped enum

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.



Comment at: clang/include/clang/Sema/DeclSpec.h:1762
 };
+using FDK = FunctionDefinitionKind;
 

aaron.ballman wrote:
> rsmith wrote:
> > I don't think it's OK to have an initialism like this in the `clang` 
> > namespace scope -- generally-speaking, the larger the scope of a name, the 
> > longer and more descriptive the name needs to be. Is spelling out the full 
> > name of the enumeration everywhere it appears unacceptably verbose?
> That will change uses like this:
> ```
> D.setFunctionDefinitionKind(FDK::Definition);
> ```
> into
> ```
> D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
> ```
> which repeats the enumeration name twice (once for the function and once for 
> the enumerator) and is rather unfortunate. I'm not certain it's more 
> unfortunate than putting an initialism into the `clang` namespace though.
Lost my original comment... I guess I still don't know how to use Phabricator :(

I see both arguments here, I think I agree more with @rsmith as I generally 
prefer less "mental indirection"/clarity over less typing. 

That said, there's also a potential middle ground here. There is a fair bit of 
inconsistency in enum naming, looking at `Specifiers.h` for instance, sometimes 
"Specifier" is spelled "Specifier" and other times it's spelled "Spec" or 
"Specifiers" (and actually looking closer, it doesn't appear that 
`TypeSpecifiersPipe` is ever used). Perhaps it would be good to standardize the 
short names, and perhaps use something like `FnDefKind` or `FunctionDefKind` -- 
both of which are notably shortly, but still reasonably understandable and 
specific names. Just a thought :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91035

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

[PATCH] D91035: [NFC, Refactor] Convert FunctionDefinitionKind from DeclSpech.h to a scoped enum

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.



Comment at: clang/include/clang/Sema/DeclSpec.h:1762
 };
+using FDK = FunctionDefinitionKind;
 

wchilders wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > I don't think it's OK to have an initialism like this in the `clang` 
> > > namespace scope -- generally-speaking, the larger the scope of a name, 
> > > the longer and more descriptive the name needs to be. Is spelling out the 
> > > full name of the enumeration everywhere it appears unacceptably verbose?
> > That will change uses like this:
> > ```
> > D.setFunctionDefinitionKind(FDK::Definition);
> > ```
> > into
> > ```
> > D.setFunctionDefinitionKind(FunctionDefinitionKind::Definition);
> > ```
> > which repeats the enumeration name twice (once for the function and once 
> > for the enumerator) and is rather unfortunate. I'm not certain it's more 
> > unfortunate than putting an initialism into the `clang` namespace though.
> Lost my original comment... I guess I still don't know how to use Phabricator 
> :(
> 
> I see both arguments here, I think I agree more with @rsmith as I generally 
> prefer less "mental indirection"/clarity over less typing. 
> 
> That said, there's also a potential middle ground here. There is a fair bit 
> of inconsistency in enum naming, looking at `Specifiers.h` for instance, 
> sometimes "Specifier" is spelled "Specifier" and other times it's spelled 
> "Spec" or "Specifiers" (and actually looking closer, it doesn't appear that 
> `TypeSpecifiersPipe` is ever used). Perhaps it would be good to standardize 
> the short names, and perhaps use something like `FnDefKind` or 
> `FunctionDefKind` -- both of which are notably shortly, but still reasonably 
> understandable and specific names. Just a thought :)
Correction `TypeSpecifiersPipe` is used, just a bit strangely -- one value, 
`TSP_pipe` is assigned to a bit field as a flag, rather than `true`.

Also notably shorter* (whoops)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91035

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

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders marked 2 inline comments as done.
wchilders added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:7329
+  if (Result.isLValue())
+return Success(Result, E);
+}

rsmith wrote:
> wchilders wrote:
> > rsmith wrote:
> > > wchilders wrote:
> > > > This doesn't seem to be the right answer, and `ConstantExpr`s don't 
> > > > have `LValue` `APValue`s, at least, not that are reaching this case. We 
> > > > had a previous implementation that also, kind of punted on this issue 
> > > > with an override in `TemporaryExprEvaluator`: 
> > > > https://gitlab.com/lock3/clang/-/blob/9fbaeea06fc567ac472264bec2a72661a1e06c73/clang/lib/AST/ExprConstant.cpp#L9753
> > > The base class version seems appropriate to me, even for this case. We 
> > > eventually want to use `ConstantExpr` to store the evaluated initializer 
> > > value of a `constexpr` variable (replacing the existing special-case 
> > > caching on `VarDecl`) and the like, not only for immediate invocations, 
> > > and once we start doing that for reference variables we'll have glvalue 
> > > `ConstantExpr`s.
> > > 
> > > Is there some circumstance under which a glvalue `ConstantExpr`'s 
> > > `APValue` result is not of kind `LValue`?
> > > Is there some circumstance under which a glvalue ConstantExpr's APValue 
> > > result is not of kind LValue?
> > 
> > Good question, the `Sema/switch.c` test fails if you 
> > `assert(Result.isLValue())`.
> > 
> > ```
> > ConstantExpr 0x62ab8760 'const int' lvalue Int: 3
> > `-DeclRefExpr 0x62ab8740 'const int' lvalue Var 0x62ab8230 'a' 
> > 'const int'
> > ```
> > 
> > With an attributed line no. 359: 
> > https://github.com/llvm/llvm-project/blob/4b0029995853fe37d1dc95ef96f46697c743fcad/clang/test/Sema/switch.c#L359
> > 
> > The offending RValue is created in SemaExpr.cpp here: 
> > https://github.com/llvm/llvm-project/blob/f87563661d6661fd4843beb8f391acd077b08dbe/clang/lib/Sema/SemaExpr.cpp#L15190
> > 
> > The issue stems from evaluating this as an RValue to produce an integer, 
> > then caching that RValue in an lvalue constant expression. Do you have any 
> > suggestions? Perhaps an LValue to RValue conversion should be performed on 
> > the expression if it folds correctly, so that the ConstantExpr is actually 
> > an RValue?
> I think we should be inserting the lvalue-to-rvalue conversion before we even 
> try evaluating the expression. Does this go wrong along both the C++11 and 
> pre-C++11 codepaths? (They do rather different conversions to the 
> expression.) In any case, we're likely missing a call to 
> `DefaultLvalueConversion` on whichever side is failing.
> 
> Judging by the fact that `struct X { void f() { switch (0) case f: ; } };` 
> misses the "non-static member function must be called" diagnostic only in 
> C++98 mode, I'd imagine it's just the pre-C++11 codepath that's broken here.
This is a C test, the C++ language level is actually irrelevant as best I can 
tell. I added the missing LValue -> RValue conversion so that the APValue is 
generated correctly. With that things function as you'd expect, I added an 
assertion to the LValue base evaluator to detect this case early in 
development, otherwise, the logic is now shared across evaluator classes.


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

https://reviews.llvm.org/D76438



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

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders updated this revision to Diff 251732.
wchilders added a comment.

Updated to assume LValue `ConstantExpr`s have LValue `APValue`s, with 
verification in debug mode. Additionally, added a missing LValue -> RValue 
conversion for `VerifyIntegerConstantExpression`.


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

https://reviews.llvm.org/D76438

Files:
  clang/include/clang/AST/Expr.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Sema/SemaExpr.cpp


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15167,6 +15167,12 @@
 return ExprError();
   }
 
+  ExprResult RValueExpr = DefaultLvalueConversion(E);
+  if (RValueExpr.isInvalid())
+return ExprError();
+
+  E = RValueExpr.get();
+
   // Circumvent ICE checking in C++11 to avoid evaluating the expression twice
   // in the non-ICE case.
   if (!getLangOpts().CPlusPlus11 && E->isIntegerConstantExpr(Context)) {
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6751,8 +6751,13 @@
 return Error(E);
   }
 
-  bool VisitConstantExpr(const ConstantExpr *E)
-{ return StmtVisitorTy::Visit(E->getSubExpr()); }
+  bool VisitConstantExpr(const ConstantExpr *E) {
+if (E->hasAPValueResult())
+  return DerivedSuccess(E->getAPValueResult(), E);
+
+return StmtVisitorTy::Visit(E->getSubExpr());
+  }
+
   bool VisitParenExpr(const ParenExpr *E)
 { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitUnaryExtension(const UnaryOperator *E)
@@ -7317,6 +7322,13 @@
 return true;
   }
 
+  // Override to perform additional checks to ensure the cached APValue
+  // is actually an LValue.
+  bool VisitConstantExpr(const ConstantExpr *E) {
+assert(!E->hasAPValueResult() || E->getAPValueResult().isLValue());
+return ExprEvaluatorBaseTy::VisitConstantExpr(E);
+  }
+
   bool VisitMemberExpr(const MemberExpr *E) {
 // Handle non-static data members.
 QualType BaseTy;
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -356,6 +356,8 @@
 }
 
 APValue ConstantExpr::getAPValueResult() const {
+  assert(hasAPValueResult());
+
   switch (ConstantExprBits.ResultKind) {
   case ConstantExpr::RSK_APValue:
 return APValueResult();
@@ -2870,9 +2872,6 @@
   return CE->getChosenSubExpr();
   }
 
-  else if (auto *CE = dyn_cast(E))
-return CE->getSubExpr();
-
   return E;
 }
 
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -1063,6 +1063,9 @@
   bool isImmediateInvocation() const {
 return ConstantExprBits.IsImmediateInvocation;
   }
+  bool hasAPValueResult() const {
+return ConstantExprBits.APValueKind != APValue::None;
+  }
   APValue getAPValueResult() const;
   APValue &getResultAsAPValue() const { return APValueResult(); }
   llvm::APSInt getResultAsAPSInt() const;


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15167,6 +15167,12 @@
 return ExprError();
   }
 
+  ExprResult RValueExpr = DefaultLvalueConversion(E);
+  if (RValueExpr.isInvalid())
+return ExprError();
+
+  E = RValueExpr.get();
+
   // Circumvent ICE checking in C++11 to avoid evaluating the expression twice
   // in the non-ICE case.
   if (!getLangOpts().CPlusPlus11 && E->isIntegerConstantExpr(Context)) {
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6751,8 +6751,13 @@
 return Error(E);
   }
 
-  bool VisitConstantExpr(const ConstantExpr *E)
-{ return StmtVisitorTy::Visit(E->getSubExpr()); }
+  bool VisitConstantExpr(const ConstantExpr *E) {
+if (E->hasAPValueResult())
+  return DerivedSuccess(E->getAPValueResult(), E);
+
+return StmtVisitorTy::Visit(E->getSubExpr());
+  }
+
   bool VisitParenExpr(const ParenExpr *E)
 { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitUnaryExtension(const UnaryOperator *E)
@@ -7317,6 +7322,13 @@
 return true;
   }
 
+  // Override to perform additional checks to ensure the cached APValue
+  // is actually an LValue.
+  bool VisitConstantExpr(const ConstantExpr *E) {
+assert(!E->hasAPValueResult() || E->getAPValueResult().isLValue());
+return ExprEvaluatorBaseTy::VisitConstantExpr(E);
+  }
+
   bool VisitMemberExpr(const MemberExpr *E) {
 // Handle non-static data members.
 QualType BaseTy;
Index: clang/lib/AST/Expr.cpp
=

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders marked 2 inline comments as done.
wchilders added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:7325-7329
+  // Override to perform additional checks to ensure the cached APValue
+  // is actually an LValue.
+  bool VisitConstantExpr(const ConstantExpr *E) {
+assert(!E->hasAPValueResult() || E->getAPValueResult().isLValue());
+return ExprEvaluatorBaseTy::VisitConstantExpr(E);

rsmith wrote:
> I think this override is now fully redundant and can be removed: the 
> `isLValue()` assert is reached anyway when `DerivedSuccess` calls 
> `LValueExprEvaluatorBase::Success` which calls `LValue::setFrom`.
Good catch, updated the patch to drop this. :)


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

https://reviews.llvm.org/D76438



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

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added a comment.

I should note, I don't have commit access, so I'm unable to commit this myself. 
Attribution would be `Wyatt Childers `


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

https://reviews.llvm.org/D76438



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

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders updated this revision to Diff 251763.
wchilders marked an inline comment as done.
wchilders added a comment.

Dropped the override for constexpr evaluator, LValue evaluation base class


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

https://reviews.llvm.org/D76438

Files:
  clang/include/clang/AST/Expr.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Sema/SemaExpr.cpp


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15167,6 +15167,12 @@
 return ExprError();
   }
 
+  ExprResult RValueExpr = DefaultLvalueConversion(E);
+  if (RValueExpr.isInvalid())
+return ExprError();
+
+  E = RValueExpr.get();
+
   // Circumvent ICE checking in C++11 to avoid evaluating the expression twice
   // in the non-ICE case.
   if (!getLangOpts().CPlusPlus11 && E->isIntegerConstantExpr(Context)) {
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6751,8 +6751,13 @@
 return Error(E);
   }
 
-  bool VisitConstantExpr(const ConstantExpr *E)
-{ return StmtVisitorTy::Visit(E->getSubExpr()); }
+  bool VisitConstantExpr(const ConstantExpr *E) {
+if (E->hasAPValueResult())
+  return DerivedSuccess(E->getAPValueResult(), E);
+
+return StmtVisitorTy::Visit(E->getSubExpr());
+  }
+
   bool VisitParenExpr(const ParenExpr *E)
 { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitUnaryExtension(const UnaryOperator *E)
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -356,6 +356,8 @@
 }
 
 APValue ConstantExpr::getAPValueResult() const {
+  assert(hasAPValueResult());
+
   switch (ConstantExprBits.ResultKind) {
   case ConstantExpr::RSK_APValue:
 return APValueResult();
@@ -2870,9 +2872,6 @@
   return CE->getChosenSubExpr();
   }
 
-  else if (auto *CE = dyn_cast(E))
-return CE->getSubExpr();
-
   return E;
 }
 
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -1063,6 +1063,9 @@
   bool isImmediateInvocation() const {
 return ConstantExprBits.IsImmediateInvocation;
   }
+  bool hasAPValueResult() const {
+return ConstantExprBits.APValueKind != APValue::None;
+  }
   APValue getAPValueResult() const;
   APValue &getResultAsAPValue() const { return APValueResult(); }
   llvm::APSInt getResultAsAPSInt() const;


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15167,6 +15167,12 @@
 return ExprError();
   }
 
+  ExprResult RValueExpr = DefaultLvalueConversion(E);
+  if (RValueExpr.isInvalid())
+return ExprError();
+
+  E = RValueExpr.get();
+
   // Circumvent ICE checking in C++11 to avoid evaluating the expression twice
   // in the non-ICE case.
   if (!getLangOpts().CPlusPlus11 && E->isIntegerConstantExpr(Context)) {
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6751,8 +6751,13 @@
 return Error(E);
   }
 
-  bool VisitConstantExpr(const ConstantExpr *E)
-{ return StmtVisitorTy::Visit(E->getSubExpr()); }
+  bool VisitConstantExpr(const ConstantExpr *E) {
+if (E->hasAPValueResult())
+  return DerivedSuccess(E->getAPValueResult(), E);
+
+return StmtVisitorTy::Visit(E->getSubExpr());
+  }
+
   bool VisitParenExpr(const ParenExpr *E)
 { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitUnaryExtension(const UnaryOperator *E)
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -356,6 +356,8 @@
 }
 
 APValue ConstantExpr::getAPValueResult() const {
+  assert(hasAPValueResult());
+
   switch (ConstantExprBits.ResultKind) {
   case ConstantExpr::RSK_APValue:
 return APValueResult();
@@ -2870,9 +2872,6 @@
   return CE->getChosenSubExpr();
   }
 
-  else if (auto *CE = dyn_cast(E))
-return CE->getSubExpr();
-
   return E;
 }
 
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -1063,6 +1063,9 @@
   bool isImmediateInvocation() const {
 return ConstantExprBits.IsImmediateInvocation;
   }
+  bool hasAPValueResult() const {
+return ConstantExprBits.APValueKind != APValue::None;
+  }
   APValue getAPValueResult() const;
   APValue &getResultAsAPValue() const { return APValueResult(); }
   llvm::APSInt getResultAsAPSInt() const;

[PATCH] D76447: Apply ConstantEvaluated evaluation contexts to more manifestly constant evaluated scopes

[Wyatt Childers via Phabricator via cfe-commits]

wchilders updated this revision to Diff 252357.
wchilders added a comment.

Updated the patch to correct formatting issues, and removed application of the 
`ConstantEvaluated` evaluation context to var decls as it introduces too much 
complexity.

Additionally removed a fix for `decltype` with immediate functions that snuck 
into this patch. This will be submitted separately.


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

https://reviews.llvm.org/D76447

Files:
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h


Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -12169,7 +12169,9 @@
   // FIXME: Sema's lambda-building mechanism expects us to push an expression
   // evaluation context even if we're not transforming the function body.
   getSema().PushExpressionEvaluationContext(
-  Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
+  E->getCallOperator()->getConstexprKind() == CSK_consteval
+  ? Sema::ExpressionEvaluationContext::ConstantEvaluated
+  : Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
 
   // Instantiate the body of the lambda expression.
   StmtResult Body =
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4644,7 +4644,9 @@
   Function->setInnerLocStart(PatternDecl->getInnerLocStart());
 
   EnterExpressionEvaluationContext EvalContext(
-  *this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
+  *this, PatternDecl->getConstexprKind() == CSK_consteval
+ ? Sema::ExpressionEvaluationContext::ConstantEvaluated
+ : Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
 
   // Introduce a new scope where local variable instantiations will be
   // recorded, unless we're actually a member function within a local
@@ -4947,8 +4949,13 @@
 Var->setImplicitlyInline();
 
   if (OldVar->getInit()) {
+bool IsConstexpr = OldVar->isConstexpr() || isConstantEvaluated();
 EnterExpressionEvaluationContext Evaluated(
-*this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated, Var);
+*this,
+getLangOpts().CPlusPlus2a && IsConstexpr
+? Sema::ExpressionEvaluationContext::ConstantEvaluated
+: Sema::ExpressionEvaluationContext::PotentiallyEvaluated,
+Var);
 
 // Instantiate the initializer.
 ExprResult Init;
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1329,10 +1329,19 @@
   // Parse the condition.
   StmtResult InitStmt;
   Sema::ConditionResult Cond;
-  if (ParseParenExprOrCondition(&InitStmt, Cond, IfLoc,
-IsConstexpr ? Sema::ConditionKind::ConstexprIf
-: Sema::ConditionKind::Boolean))
-return StmtError();
+
+  {
+EnterExpressionEvaluationContext Unevaluated(
+Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
+/*LambdaContextDecl=*/nullptr, /*ExprContext=*/
+Sema::ExpressionEvaluationContextRecord::EK_Other,
+/*ShouldEnter=*/IsConstexpr);
+
+if (ParseParenExprOrCondition(&InitStmt, Cond, IfLoc,
+  IsConstexpr ? 
Sema::ConditionKind::ConstexprIf
+  : Sema::ConditionKind::Boolean))
+  return StmtError();
+  }
 
   llvm::Optional ConstexprCondition;
   if (IsConstexpr)
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -2974,11 +2974,20 @@
 /// be a constant-expression.
 ExprResult Parser::ParseCXXMemberInitializer(Decl *D, bool IsFunction,
  SourceLocation &EqualLoc) {
+  using EEC = Sema::ExpressionEvaluationContext;
+
   assert(Tok.isOneOf(tok::equal, tok::l_brace)
  && "Data member initializer not starting with '=' or '{'");
 
+  bool IsConstexpr = false;
+  if (auto *VD = dyn_cast_or_null(D))
+IsConstexpr = VD->isConstexpr();
+
   EnterExpressionEvaluationContext Context(
-  Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated, D);
+  Actions,
+  getLangOpts().CPlusPlus2a && IsConstexpr ? EEC::ConstantEvaluated
+   : EEC::PotentiallyEvaluated,
+  D);
   if (TryConsumeToken(tok::equal, EqualLoc)) {
 if (Tok.is(tok::kw_delete)) {
   // In principle, an initializer of '= delete p;' is legal, but it will


Index: clang

[PATCH] D76724: Prevent immediate evaluations inside of decltype

[Wyatt Childers via Phabricator via cfe-commits]

wchilders created this revision.
wchilders added reviewers: Tyker, rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This fixes an issue where immediate invocations were queued inside of decltype, 
resulting in decltype's operand being evaluated.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76724

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/decltype-consteval.cpp


Index: clang/test/SemaCXX/decltype-consteval.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/decltype-consteval.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++2a %s
+// expected-no-diagnostics
+
+struct MaybeConsteval {
+  MaybeConsteval* ptr = nullptr;
+
+  consteval MaybeConsteval(bool Valid) : ptr(this) {
+if (Valid) {
+  ptr = nullptr;
+}
+  }
+};
+
+consteval decltype(MaybeConsteval(false)) foo() {
+  return { true };
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15377,9 +15377,16 @@
   }
 }
 
+// Check to see if this expression is part of a decltype specifier.
+// We're not interested in evaluating this expression, immediately or 
otherwise.
+static bool isInDeclType(Sema &SemaRef) {
+  return SemaRef.ExprEvalContexts.back().ExprContext ==
+ Sema::ExpressionEvaluationContextRecord::EK_Decltype;
+}
+
 ExprResult Sema::CheckForImmediateInvocation(ExprResult E, FunctionDecl *Decl) 
{
   if (!E.isUsable() || !Decl || !Decl->isConsteval() || isConstantEvaluated() 
||
-  RebuildingImmediateInvocation)
+  isInDeclType(*this) || RebuildingImmediateInvocation)
 return E;
 
   /// Opportunistically remove the callee from ReferencesToConsteval if we can.


Index: clang/test/SemaCXX/decltype-consteval.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/decltype-consteval.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++2a %s
+// expected-no-diagnostics
+
+struct MaybeConsteval {
+  MaybeConsteval* ptr = nullptr;
+
+  consteval MaybeConsteval(bool Valid) : ptr(this) {
+if (Valid) {
+  ptr = nullptr;
+}
+  }
+};
+
+consteval decltype(MaybeConsteval(false)) foo() {
+  return { true };
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15377,9 +15377,16 @@
   }
 }
 
+// Check to see if this expression is part of a decltype specifier.
+// We're not interested in evaluating this expression, immediately or otherwise.
+static bool isInDeclType(Sema &SemaRef) {
+  return SemaRef.ExprEvalContexts.back().ExprContext ==
+ Sema::ExpressionEvaluationContextRecord::EK_Decltype;
+}
+
 ExprResult Sema::CheckForImmediateInvocation(ExprResult E, FunctionDecl *Decl) {
   if (!E.isUsable() || !Decl || !Decl->isConsteval() || isConstantEvaluated() ||
-  RebuildingImmediateInvocation)
+  isInDeclType(*this) || RebuildingImmediateInvocation)
 return E;
 
   /// Opportunistically remove the callee from ReferencesToConsteval if we can.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76443: Use ConstantExpr cached APValues if present for code generation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders updated this revision to Diff 252610.
wchilders added a comment.

Updated to remove duplicated (and out dated) APValue generation logic, and 
fixed formatting issues. (Thanks for pointing out something was awry Tyker)


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

https://reviews.llvm.org/D76443

Files:
  clang/include/clang/AST/Expr.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmt.cpp

Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -1092,8 +1092,13 @@
   // statements following block literals with non-trivial cleanups.
   RunCleanupsScope cleanupScope(*this);
   if (const FullExpr *fe = dyn_cast_or_null(RV)) {
-enterFullExpression(fe);
-RV = fe->getSubExpr();
+// Constant expressions will emit their cached APValues, so
+// we don't want to handle this here, as it will unwrap them,
+// preventing the cached results from being used.
+if (!isa(fe)) {
+  enterFullExpression(fe);
+  RV = fe->getSubExpr();
+}
   }
 
   // FIXME: Clean this up by using an LValue for ReturnTemp,
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -433,7 +433,13 @@
   Value *VisitExpr(Expr *S);
 
   Value *VisitConstantExpr(ConstantExpr *E) {
-return Visit(E->getSubExpr());
+if (E->hasAPValueResult()) {
+  assert(!E->getType()->isVoidType());
+  return ConstantEmitter(CGF).emitAbstract(
+  E->getLocation(), E->getAPValueResult(), E->getType());
+} else {
+  return Visit(E->getSubExpr());
+}
   }
   Value *VisitParenExpr(ParenExpr *PE) {
 return Visit(PE->getSubExpr());
Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -126,7 +126,17 @@
   }
 
   void VisitConstantExpr(ConstantExpr *E) {
-return Visit(E->getSubExpr());
+if (E->hasAPValueResult()) {
+  // Create a temporary for the value and store the constant.
+  llvm::Constant *Const = ConstantEmitter(CGF).emitAbstract(
+  E->getLocation(), E->getAPValueResult(), E->getType());
+  Address Addr = CGF.CreateMemTemp(E->getType());
+  CGF.InitTempAlloca(Addr, Const);
+  RValue RV = RValue::getAggregate(Addr);
+  EmitFinalDestCopy(E->getType(), RV);
+} else {
+  Visit(E->getSubExpr());
+}
   }
 
   // l-values.
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -1292,8 +1292,20 @@
 return EmitVAArgExprLValue(cast(E));
   case Expr::DeclRefExprClass:
 return EmitDeclRefLValue(cast(E));
-  case Expr::ConstantExprClass:
-return EmitLValue(cast(E)->getSubExpr());
+  case Expr::ConstantExprClass: {
+const ConstantExpr *CE = cast(E);
+
+if (CE->hasAPValueResult()) {
+  QualType T = getContext().getPointerType(CE->getType());
+  llvm::Constant *C = ConstantEmitter(*this).emitAbstract(
+  CE->getLocation(), CE->getAPValueResult(), T);
+  ConstantAddress Addr(C, getContext().getTypeAlignInChars(T));
+  LValueBaseInfo BI;
+  return LValue::MakeAddr(Addr, T, getContext(), BI, TBAAAccessInfo());
+}
+
+return EmitLValue(CE->getSubExpr());
+  }
   case Expr::ParenExprClass:
 return EmitLValue(cast(E)->getSubExpr());
   case Expr::GenericSelectionExprClass:
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -1031,6 +1031,7 @@
   static ResultStorageKind getStorageKind(const Type *T,
   const ASTContext &Context);
 
+  SourceLocation getLocation() const { return getBeginLoc(); }
   SourceLocation getBeginLoc() const LLVM_READONLY {
 return SubExpr->getBeginLoc();
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76724: Prevent immediate evaluations inside of decltype

[Wyatt Childers via Phabricator via cfe-commits]

wchilders abandoned this revision.
wchilders added a comment.

Thanks Tyker, you're right, there is a general problem that needs handled, if 
this is to be handled; I had a bit of tunnel vision here. However, I'm 
retracting/abandoning this patch entirely as upon further review, it's not 
standard compliant. The current situation, is actually the correct one (for 
now?).

From [7.7.13] of the working draft:

> Note: A manifestly constant-evaluated expression is evaluated even in an 
> unevaluated operand.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76724



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

[PATCH] D76396: Allow immediate invocation of constructors

[Wyatt Childers via Phabricator via cfe-commits]

wchilders created this revision.
wchilders added reviewers: rsmith, Tyker.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
wchilders retitled this revision from "Allow immediate invocations of 
constructors" to "Allow immediate invocation of constructors".

Hi all,

This is an initial patch submission falling out of Lock3's reflection and 
metaprogramming work. This is a rather small patch to allow immediate 
invocation of constructors.

We have additional larger patches for consteval which will likely be a bit 
trickier, so I wanted to start here to help familiarize myself with the process.

For context, we have forthcoming patches that:

- Provide improvements which allow the cached values of ConstantExpr to be used 
by both the constant evaluator and code gen
- Update the application of evaluation contexts, using the stronger guarantees 
of manifest constant evaluation to apply the ConstantEvaluated evaluation 
context in more places

These are both larger, and while we depend on them in observable ways for our 
work, these are not currently observable changes from the perspective of the 
test suite.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76396

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -441,3 +441,14 @@
 };
   }
 }
+
+namespace constructor {
+  struct A {
+A* ptr = nullptr;
+consteval A() : ptr(this) { }
+  };
+
+  A a = A(); // expected-error {{is not a constant expression}}
+  // expected-note@-1 {{is not a constant expression}} expected-note@-1 
{{temporary created here}}
+}
+
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6500,7 +6500,7 @@
   if (shouldBindAsTemporary(Entity))
 CurInit = S.MaybeBindToTemporary(CurInit.get());
 
-  return CurInit;
+  return S.CheckForImmediateInvocation(CurInit, Constructor);
 }
 
 namespace {


Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -441,3 +441,14 @@
 };
   }
 }
+
+namespace constructor {
+  struct A {
+A* ptr = nullptr;
+consteval A() : ptr(this) { }
+  };
+
+  A a = A(); // expected-error {{is not a constant expression}}
+  // expected-note@-1 {{is not a constant expression}} expected-note@-1 {{temporary created here}}
+}
+
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6500,7 +6500,7 @@
   if (shouldBindAsTemporary(Entity))
 CurInit = S.MaybeBindToTemporary(CurInit.get());
 
-  return CurInit;
+  return S.CheckForImmediateInvocation(CurInit, Constructor);
 }
 
 namespace {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76396: Allow immediate invocation of constructors

[Wyatt Childers via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaInit.cpp:6466
 if (Entity.allowsNRVO())
   CurInit = S.BuildCXXConstructExpr(Loc, Step.Type,
 Step.Function.FoundDecl,

rsmith wrote:
> It looks like the other callers to `BuildCXXConstructExpr` are also missing 
> this handling. Can we put the call to `CheckForImmediateInvocation` in 
> `BuildCXXConstructExpr` to handle all those cases at once, or do we need to 
> defer it until after the other stuff below?
Those changes shouldn't have impact here. 

I choose to apply this here as when looking at other uses of 
`CheckForImmediateInvocation`, `MaybeBindToTemporary` was performed on the 
expr, prior to the expr being passed to `CheckForImmediateInvocation`.

Upon changing this over, all tests (upstream and ours) still pass, so if you 
don't see any issues, I'm fine with moving this into `BuildCXXConstructExpr`. 
Additionally, upon closer inspection, there is low coupling between 
`MaybeBindToTemporary` and `CXXConstructExpr`, where as there is significant 
coupling for ObjC and `CallExpr`s. Unless something I've said here gives you 
some doubts, I'm fine with moving this to `BuildCXXConstructExpr`, there's 
nothing that immediately jumps out to me as a problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76396



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

[PATCH] D76396: Allow immediate invocation of constructors

[Wyatt Childers via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaInit.cpp:6466
 if (Entity.allowsNRVO())
   CurInit = S.BuildCXXConstructExpr(Loc, Step.Type,
 Step.Function.FoundDecl,

wchilders wrote:
> rsmith wrote:
> > It looks like the other callers to `BuildCXXConstructExpr` are also missing 
> > this handling. Can we put the call to `CheckForImmediateInvocation` in 
> > `BuildCXXConstructExpr` to handle all those cases at once, or do we need to 
> > defer it until after the other stuff below?
> Those changes shouldn't have impact here. 
> 
> I choose to apply this here as when looking at other uses of 
> `CheckForImmediateInvocation`, `MaybeBindToTemporary` was performed on the 
> expr, prior to the expr being passed to `CheckForImmediateInvocation`.
> 
> Upon changing this over, all tests (upstream and ours) still pass, so if you 
> don't see any issues, I'm fine with moving this into `BuildCXXConstructExpr`. 
> Additionally, upon closer inspection, there is low coupling between 
> `MaybeBindToTemporary` and `CXXConstructExpr`, where as there is significant 
> coupling for ObjC and `CallExpr`s. Unless something I've said here gives you 
> some doubts, I'm fine with moving this to `BuildCXXConstructExpr`, there's 
> nothing that immediately jumps out to me as a problem.
Upon changing this over, all tests (upstream and ours) still pass~~, so if you 
don't see any issues, I'm fine with moving this into BuildCXXConstructExpr~~.  
(Whoops, rearranged some sentences last second heh)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76396



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

[PATCH] D76396: Allow immediate invocation of constructors

[Wyatt Childers via Phabricator via cfe-commits]

wchilders abandoned this revision.
wchilders added a comment.

In D76396#1930773 , @Tyker wrote:

> I have already a patch aiming to do the same thing. D74007 
> 


Oof, okay, sounds good. I was not aware of this patch, it looks to be much 
further along :)

>> Provide improvements which allow the cached values of ConstantExpr to be 
>> used by both the constant evaluator and code gen
> 
> this is definitely something we should do.
> 
>> Update the application of evaluation contexts, using the stronger guarantees 
>> of manifest constant evaluation to apply the ConstantEvaluated evaluation 
>> context in more places
> 
> the tracking of evaluation context isn't as good as it could/should.

With this being closed out in favor of Tyker's patch, I'll post a patch for 
reusing cached values of ConstantExpr shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76396



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

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders created this revision.
wchilders added reviewers: Tyker, rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch allows the constant evaluator to use `APValueResult`s from 
`ConstantExpr`s.

There are some outstanding concerns I'll mark with inline comments.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76438

Files:
  clang/include/clang/AST/Expr.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprConstant.cpp


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6751,8 +6751,13 @@
 return Error(E);
   }
 
-  bool VisitConstantExpr(const ConstantExpr *E)
-{ return StmtVisitorTy::Visit(E->getSubExpr()); }
+  bool VisitConstantExpr(const ConstantExpr *E) {
+if (E->hasAPValueResult())
+  return DerivedSuccess(E->getAPValueResult(), E);
+
+return StmtVisitorTy::Visit(E->getSubExpr());
+  }
+
   bool VisitParenExpr(const ParenExpr *E)
 { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitUnaryExtension(const UnaryOperator *E)
@@ -7317,6 +7322,16 @@
 return true;
   }
 
+  bool VisitConstantExpr(const ConstantExpr *E) {
+if (E->hasAPValueResult()) {
+  APValue Result = E->getAPValueResult();
+  if (Result.isLValue())
+return Success(Result, E);
+}
+
+return ExprEvaluatorBaseTy::Visit(E->getSubExpr());
+  }
+
   bool VisitMemberExpr(const MemberExpr *E) {
 // Handle non-static data members.
 QualType BaseTy;
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -356,6 +356,8 @@
 }
 
 APValue ConstantExpr::getAPValueResult() const {
+  assert(hasAPValueResult());
+
   switch (ConstantExprBits.ResultKind) {
   case ConstantExpr::RSK_APValue:
 return APValueResult();
@@ -2870,9 +2872,6 @@
   return CE->getChosenSubExpr();
   }
 
-  else if (auto *CE = dyn_cast(E))
-return CE->getSubExpr();
-
   return E;
 }
 
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -1063,6 +1063,9 @@
   bool isImmediateInvocation() const {
 return ConstantExprBits.IsImmediateInvocation;
   }
+  bool hasAPValueResult() const {
+return ConstantExprBits.APValueKind != APValue::None;
+  }
   APValue getAPValueResult() const;
   APValue &getResultAsAPValue() const { return APValueResult(); }
   llvm::APSInt getResultAsAPSInt() const;


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -6751,8 +6751,13 @@
 return Error(E);
   }
 
-  bool VisitConstantExpr(const ConstantExpr *E)
-{ return StmtVisitorTy::Visit(E->getSubExpr()); }
+  bool VisitConstantExpr(const ConstantExpr *E) {
+if (E->hasAPValueResult())
+  return DerivedSuccess(E->getAPValueResult(), E);
+
+return StmtVisitorTy::Visit(E->getSubExpr());
+  }
+
   bool VisitParenExpr(const ParenExpr *E)
 { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitUnaryExtension(const UnaryOperator *E)
@@ -7317,6 +7322,16 @@
 return true;
   }
 
+  bool VisitConstantExpr(const ConstantExpr *E) {
+if (E->hasAPValueResult()) {
+  APValue Result = E->getAPValueResult();
+  if (Result.isLValue())
+return Success(Result, E);
+}
+
+return ExprEvaluatorBaseTy::Visit(E->getSubExpr());
+  }
+
   bool VisitMemberExpr(const MemberExpr *E) {
 // Handle non-static data members.
 QualType BaseTy;
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -356,6 +356,8 @@
 }
 
 APValue ConstantExpr::getAPValueResult() const {
+  assert(hasAPValueResult());
+
   switch (ConstantExprBits.ResultKind) {
   case ConstantExpr::RSK_APValue:
 return APValueResult();
@@ -2870,9 +2872,6 @@
   return CE->getChosenSubExpr();
   }
 
-  else if (auto *CE = dyn_cast(E))
-return CE->getSubExpr();
-
   return E;
 }
 
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -1063,6 +1063,9 @@
   bool isImmediateInvocation() const {
 return ConstantExprBits.IsImmediateInvocation;
   }
+  bool hasAPValueResult() const {
+return ConstantExprBits.APValueKind != APValue::None;
+  }
   APValue getAPValueResult() const;
   APValue &getResultAsAPValue() const { return APValueResult(); }
   llvm::APSInt getResultAsAPSInt() const;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.



Comment at: clang/lib/AST/Expr.cpp:2875
 
-  else if (auto *CE = dyn_cast(E))
-return CE->getSubExpr();
-
   return E;
 }

`IgnoreParensSingleStep` for some reason has been unwrapping `ConstantExpr`s. 
This results in the constant evaluator, removing the ConstantExpr, and 
reevaluating the expression. There are no observed downsides to removing this 
condition, in the test suite, however, it's strange enough to note.



Comment at: clang/lib/AST/ExprConstant.cpp:7329
+  if (Result.isLValue())
+return Success(Result, E);
+}

This doesn't seem to be the right answer, and `ConstantExpr`s don't have 
`LValue` `APValue`s, at least, not that are reaching this case. We had a 
previous implementation that also, kind of punted on this issue with an 
override in `TemporaryExprEvaluator`: 
https://gitlab.com/lock3/clang/-/blob/9fbaeea06fc567ac472264bec2a72661a1e06c73/clang/lib/AST/ExprConstant.cpp#L9753


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76438



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

[PATCH] D76443: Use ConstantExpr cached APValues if present for code generation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders created this revision.
wchilders added reviewers: void, Tyker, rsmith.
Herald added subscribers: cfe-commits, jfb.
Herald added a project: clang.
wchilders added inline comments.



Comment at: clang/lib/CodeGen/CGStmt.cpp:1095
+// preventing the cached results from being used.
+if (!isa(fe)) {
+  enterFullExpression(fe);

This one is a bit weird, I think the comment explains it well, but I feel there 
should be a better way to handle this or if there are similar issues lurking 
elsewhere.


This patch allows code gen to use `APValue` Results from `ConstantExpr`s.

There are some outstanding concerns I'll mark with inline comments.

This is the codegen "sister patch" of: https://reviews.llvm.org/D76438


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76443

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.h

Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1390,6 +1390,21 @@
   /// \param QT is the clang QualType of the null pointer.
   llvm::Constant *getNullPointer(llvm::PointerType *T, QualType QT);
 
+  /// Try to emit the given expression as a constant; returns 0 if the
+  /// expression cannot be emitted as a constant.
+  llvm::Constant *EmitConstantExpr(const Expr *E, QualType DestType,
+   CodeGenFunction *CGF = nullptr);
+
+  /// Emit the given constant value as a constant, in the type's scalar
+  /// representation.
+  llvm::Constant *EmitConstantValue(const APValue &Value, QualType DestType,
+CodeGenFunction *CGF = nullptr);
+
+  /// Emit the given constant value as a constant, in the type's memory
+  /// representation.
+  llvm::Constant *EmitConstantValueForMemory(const APValue &Value,
+ QualType DestType,
+ CodeGenFunction *CGF = nullptr);
 private:
   llvm::Constant *GetOrCreateLLVMFunction(
   StringRef MangledName, llvm::Type *Ty, GlobalDecl D, bool ForVTable,
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -2289,6 +2289,8 @@
 TBAAAccessInfo *TBAAInfo = nullptr);
   LValue EmitLoadOfPointerLValue(Address Ptr, const PointerType *PtrTy);
 
+  llvm::Constant *EmitConstantValue(const APValue &Value, QualType DestType);
+
   /// CreateTempAlloca - This creates an alloca and inserts it into the entry
   /// block if \p ArraySize is nullptr, otherwise inserts it at the current
   /// insertion point of the builder. The caller is responsible for setting an
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2529,3 +2529,9 @@
 
   return llvm::DebugLoc();
 }
+
+
+llvm::Constant *CodeGenFunction::EmitConstantValue(const APValue& Value,
+   QualType DestType) {
+  return CGM.EmitConstantValue(Value, DestType, this);
+}
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -1089,8 +1089,13 @@
   // statements following block literals with non-trivial cleanups.
   RunCleanupsScope cleanupScope(*this);
   if (const FullExpr *fe = dyn_cast_or_null(RV)) {
-enterFullExpression(fe);
-RV = fe->getSubExpr();
+// Constant expressions will emit their cached APValues, so
+// we don't want to handle this here, as it will unwrap them,
+// preventing the cached results from being used.
+if (!isa(fe)) {
+  enterFullExpression(fe);
+  RV = fe->getSubExpr();
+}
   }
 
   // FIXME: Clean this up by using an LValue for ReturnTemp,
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -433,7 +433,12 @@
   Value *VisitExpr(Expr *S);
 
   Value *VisitConstantExpr(ConstantExpr *E) {
-return Visit(E->getSubExpr());
+if (E->hasAPValueResult()) {
+  assert(!E->getType()->isVoidType());
+  return CGF.EmitConstantValue(E->getAPValueResult(), E->getType());
+} else {
+  return Visit(E->getSubExpr());
+}
   }
   Value *VisitParenExpr(ParenExpr *PE) {
 return Visit(PE->getSubExpr());
Index: clang/lib/CodeGen/CGExprConstant.cpp
=

[PATCH] D76443: Use ConstantExpr cached APValues if present for code generation

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.



Comment at: clang/lib/CodeGen/CGStmt.cpp:1095
+// preventing the cached results from being used.
+if (!isa(fe)) {
+  enterFullExpression(fe);

This one is a bit weird, I think the comment explains it well, but I feel there 
should be a better way to handle this or if there are similar issues lurking 
elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76443



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

[PATCH] D76447: Apply ConstantEvaluated evaluation contexts to more manifestly constant evaluated scopes

[Wyatt Childers via Phabricator via cfe-commits]

wchilders created this revision.
wchilders added reviewers: Tyker, rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
wchilders marked an inline comment as done.
wchilders added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15597
+  (Rec.isConstantEvaluated() &&
+   Rec.ExprContext != ExpressionKind::EK_ConstexprVarInit)) {
 ExprCleanupObjects.erase(ExprCleanupObjects.begin() + 
Rec.NumCleanupObjects,

This is my main concern with this patch. There's something subtle here, and 
it's unclear from an outsider's (my) perspective what the root issue is here.

Effectively we get into trouble when we first enter 
`ActOnCXXEnterDeclInitializer` on the `isManifestlyEvaluatedVar` branch. We 
then exit at `ActOnCXXExitDeclInitializer`, triggering this branch, as 
`Rec.isConstantEvaluated()` is true. This results in the cleanups being 
dropped. Then when we enter the constexpr evaluator for 
`VarDecl::evaluateValue`. the evaluator is unhappy as we're missing a cleanup 
for the created temporary.

Any input that allows this to be resolved without the special case, or that 
otherwise informs how this should work, and why unevaluated and constant 
evaluated are treated equally here, would be awesome. I'll note that in 
reviewing the revision history, this condition dates back to when 
`ConstantEvaluated` was created, so perhaps this is a "historical artifact" of 
that time?


This patch attempts to update application of evaluation contexts to better 
represent, what is manifestly constant evaluated.

There are some outstanding concerns I'll mark with inline comments.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76447

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h

Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -12079,6 +12079,8 @@
   // FIXME: Sema's lambda-building mechanism expects us to push an expression
   // evaluation context even if we're not transforming the function body.
   getSema().PushExpressionEvaluationContext(
+  E->getCallOperator()->getConstexprKind() == CSK_consteval ?
+  Sema::ExpressionEvaluationContext::ConstantEvaluated :
   Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
 
   // Instantiate the body of the lambda expression.
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4644,7 +4644,10 @@
   Function->setInnerLocStart(PatternDecl->getInnerLocStart());
 
   EnterExpressionEvaluationContext EvalContext(
-  *this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
+  *this,
+  PatternDecl->getConstexprKind() == CSK_consteval
+  ? Sema::ExpressionEvaluationContext::ConstantEvaluated
+  : Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
 
   // Introduce a new scope where local variable instantiations will be
   // recorded, unless we're actually a member function within a local
@@ -4947,8 +4950,11 @@
 Var->setImplicitlyInline();
 
   if (OldVar->getInit()) {
+bool IsConstexpr = OldVar->isConstexpr() || isConstantEvaluated();
 EnterExpressionEvaluationContext Evaluated(
-*this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated, Var);
+*this, getLangOpts().CPlusPlus2a && IsConstexpr ?
+Sema::ExpressionEvaluationContext::ConstantEvaluated :
+Sema::ExpressionEvaluationContext::PotentiallyEvaluated, Var);
 
 // Instantiate the initializer.
 ExprResult Init;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15369,9 +15369,16 @@
   }
 }
 
+// Check to see if this expression is part of a decltype specifier.
+// We're not interested in evaluating this expression, immediately or otherwise.
+static bool isInDeclType(Sema &SemaRef) {
+  return SemaRef.ExprEvalContexts.back().ExprContext ==
+Sema::ExpressionEvaluationContextRecord::EK_Decltype;
+}
+
 ExprResult Sema::CheckForImmediateInvocation(ExprResult E, FunctionDecl *Decl) {
   if (!E.isUsable() || !Decl || !Decl->isConsteval() || isConstantEvaluated() ||
-  RebuildingImmediateInvocation)
+  isInDeclType(*this) || RebuildingImmediateInvocation)
 return E;
 
   /// Opportunistically remove the callee from ReferencesToConsteval if we can.
@@ -15532,11 +15539,12 @@
 }
 
 void Sema::PopExpressionEvaluationContext() {
+  using ExpressionKind = ExpressionEvaluationContextRecord::ExpressionK

[PATCH] D76447: Apply ConstantEvaluated evaluation contexts to more manifestly constant evaluated scopes

[Wyatt Childers via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaExpr.cpp:15597
+  (Rec.isConstantEvaluated() &&
+   Rec.ExprContext != ExpressionKind::EK_ConstexprVarInit)) {
 ExprCleanupObjects.erase(ExprCleanupObjects.begin() + 
Rec.NumCleanupObjects,

This is my main concern with this patch. There's something subtle here, and 
it's unclear from an outsider's (my) perspective what the root issue is here.

Effectively we get into trouble when we first enter 
`ActOnCXXEnterDeclInitializer` on the `isManifestlyEvaluatedVar` branch. We 
then exit at `ActOnCXXExitDeclInitializer`, triggering this branch, as 
`Rec.isConstantEvaluated()` is true. This results in the cleanups being 
dropped. Then when we enter the constexpr evaluator for 
`VarDecl::evaluateValue`. the evaluator is unhappy as we're missing a cleanup 
for the created temporary.

Any input that allows this to be resolved without the special case, or that 
otherwise informs how this should work, and why unevaluated and constant 
evaluated are treated equally here, would be awesome. I'll note that in 
reviewing the revision history, this condition dates back to when 
`ConstantEvaluated` was created, so perhaps this is a "historical artifact" of 
that time?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76447



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

[PATCH] D76438: ConstantExpr cached APValues if present for constant evaluation

[Wyatt Childers via Phabricator via cfe-commits]

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



Comment at: clang/lib/AST/ExprConstant.cpp:7329
+  if (Result.isLValue())
+return Success(Result, E);
+}

rsmith wrote:
> wchilders wrote:
> > This doesn't seem to be the right answer, and `ConstantExpr`s don't have 
> > `LValue` `APValue`s, at least, not that are reaching this case. We had a 
> > previous implementation that also, kind of punted on this issue with an 
> > override in `TemporaryExprEvaluator`: 
> > https://gitlab.com/lock3/clang/-/blob/9fbaeea06fc567ac472264bec2a72661a1e06c73/clang/lib/AST/ExprConstant.cpp#L9753
> The base class version seems appropriate to me, even for this case. We 
> eventually want to use `ConstantExpr` to store the evaluated initializer 
> value of a `constexpr` variable (replacing the existing special-case caching 
> on `VarDecl`) and the like, not only for immediate invocations, and once we 
> start doing that for reference variables we'll have glvalue `ConstantExpr`s.
> 
> Is there some circumstance under which a glvalue `ConstantExpr`'s `APValue` 
> result is not of kind `LValue`?
> Is there some circumstance under which a glvalue ConstantExpr's APValue 
> result is not of kind LValue?

Good question, the `Sema/switch.c` test fails if you 
`assert(Result.isLValue())`.

```
ConstantExpr 0x62ab8760 'const int' lvalue Int: 3
`-DeclRefExpr 0x62ab8740 'const int' lvalue Var 0x62ab8230 'a' 'const 
int'
```

With an attributed line no. 359: 
https://github.com/llvm/llvm-project/blob/4b0029995853fe37d1dc95ef96f46697c743fcad/clang/test/Sema/switch.c#L359

The offending RValue is created in SemaExpr.cpp here: 
https://github.com/llvm/llvm-project/blob/f87563661d6661fd4843beb8f391acd077b08dbe/clang/lib/Sema/SemaExpr.cpp#L15190

The issue stems from evaluating this as an RValue to produce an integer, then 
caching that RValue in an lvalue constant expression. Do you have any 
suggestions? Perhaps an LValue to RValue conversion should be performed on the 
expression if it folds correctly, so that the ConstantExpr is actually an 
RValue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76438



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

[PATCH] D76447: Apply ConstantEvaluated evaluation contexts to more manifestly constant evaluated scopes

[Wyatt Childers via Phabricator via cfe-commits]

wchilders added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16793-16797
+  if (isManifestlyEvaluatedVar(*this, D)) {
+using ExpressionKind = ExpressionEvaluationContextRecord::ExpressionKind;
+
+PushExpressionEvaluationContext(
+ExpressionEvaluationContext::ConstantEvaluated, D, 
ExpressionKind::EK_ConstexprVarInit);

rsmith wrote:
> We can't implement the checks for manifestly constant-evaluated initializers 
> this way in general. Per [expr.const]/14, we need to treat the initializer as 
> manifestly constant-evaluated if it's "the initializer of a variable that is 
> usable in constant expressions or has constant initialization". We can't test 
> either of those conditions in general until we know what the initializer is, 
> because they can both depend on whether evaluation of the initializer 
> succeeds. (This approach works for the case of `constexpr` variables, which 
> are always usable in constant expressions, but not any of the other cases, 
> and the approach we'll need for the other cases will also handle `constexpr` 
> variables. There is a very modest benefit to special-casing `constexpr` 
> variable initializers regardless -- we can avoid forming and then pruning out 
> nested `ConstantExpr` nodes for immediate invocations inside the initializer 
> -- but I think it's probably not worth the added complexity.)
> 
> To address the general problem, we should handle this in 
> `CheckCompleteVariableDeclaration`, which is where we evaluate the 
> initializer and generally determine whether the variable has constant 
> initialization and / or is usable in constant expressions. Probably the 
> cleanest approach -- and certainly the one I'd been intending to pursue -- 
> would be to wrap the initializer with a `ConstantExpr` there in the relevant 
> cases, and allow the usual handling of nested immediate invocations to prune 
> out any `ConstantExpr`s nested within the initializer representing inner 
> calls to `consteval` functions. (I think I've mentioned elsewhere that I 
> would like to remove the "evaluated value" storage on `VarDecl` in favor of 
> using `ConstantExpr` for this purpose.)
> There is a very modest benefit to special-casing constexpr variable 
> initializers regardless -- we can avoid forming and then pruning out nested 
> ConstantExpr nodes for immediate invocations inside the initializer -- but I 
> think it's probably not worth the added complexity.

So, this patch is motivated for us by the desire to check if a "meta type" 
variable, belongs to a compile time, or runtime. Additionally, this is used 
being used to verify our compile time, "injection statement", is not appearing 
in a runtime context. This prevents reflections/metaprogramming values and 
logic from leaking nonsensically into runtime code. 

I think this can be accomplished as you said in the 
`CheckCompleteVariableDeclaration`. We're already checking for out of place 
"meta type" variables there, though we catch fragments, and injection 
statements more eagerly. In retrospect, I don't think there is any issue 
removing the eager check from the fragments, and I don't think the checking of 
the injection statements should be affected by this case.

I'll try and look more into this in the morning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76447



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