[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivateForVarInit try ConstExprEmitter fast-path first

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
     // This is a string literal initializing an array in an initializer.
-    return CGM.GetConstantArrayFromStringLiteral(E);
+    return E->isLValue() ?
+      CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > efriedma wrote:
> > > > > Maybe we should have a separate ConstExprEmitterLValue... trying to handle both LValues and RValues on the same codepath has been problematic in the past.  It's very easy for code to get confused what it's actually trying to emit.
> > > > So we'd have a `ConstExprEmitterLValue` class with some visitor methods, and a `ConstExprEmitterRValue` with other methods implemented?
> > > Something like that.
> > > 
> > > Actually thinking about it a bit more, not sure you need to actually implement ConstExprEmitterLValue for now.  You might just be able to ensure we don't ever call ConstExprEmitter with an lvalue.  The current ConstExprEmitter doesn't expect lvalues, and shouldn't call itself with lvalues.  (We bail on explicit LValueToRValue conversions.)  And Evaluate() shouldn't actually evaluate the contents of an lvalue if it isn't dereferenced, so there hopefully aren't any performance issues using that codepath.
> > > 
> > > In terms of implementation, I guess that's basically restoring the destType->isReferenceType() that got removed?  (I know I suggested it, but I wasn't really thinking about it...)
> > One thing I think we may need to add to `ConstExprEmitter` is the ability to evaluate `CallExpr`s based on certain test case failures...does that seem right?
> See also the calls to `constexpr f()` in clang/test/CodeGenCXX/const-init-cxx1y.cpp
The only things I know of that Evaluate() can't handle are CK_ToUnion, CK_ReinterpretMemberPointer, and DesignatedInitUpdateExpr.  DesignatedInitUpdateExpr is related to the failures in test/CodeGenCXX/designated-init.cpp; I don't think the others show up in any of the testcases you've mentioned.  (CK_ToUnion can't appear in C++ code. CK_ReinterpretMemberPointer is a `reinterpret_cast<T>` where T is a member pointer type.)

Given none of those constructs show up in const-init-cxx1y.cpp, the only reason for it to fail is if we aren't correctly falling back for a construct the current code doesn't know how to handle.  You shouldn't need to implement any new constructs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587