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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Two points I'm not sure about in the current version:

- Handling MaterializeTemporaryExpr in ConstExprEmitter doesn't make sense; ConstExprEmitter is not supposed to visit lvalues. (And I'm not sure what the new check is supposed to do; `E->isGLValue()` is always true for a MaterializeTemporaryExpr.)  Maybe try something like the following?

  diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
  index b0dd598..51cf69e 100644
  --- a/clang/lib/CodeGen/CGExprConstant.cpp
  +++ b/clang/lib/CodeGen/CGExprConstant.cpp
  @@ -1215,11 +1215,6 @@ public:
       return Visit(E->getSubExpr(), T);
     }
  
  -  llvm::Constant *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *E,
  -                                                QualType T) {
  -    return Visit(E->getSubExpr(), T);
  -  }
  -
     llvm::Constant *EmitArrayInitialization(InitListExpr *ILE, QualType T) {
       auto *CAT = CGM.getContext().getAsConstantArrayType(ILE->getType());
       assert(CAT && "can't emit array init for non-constant-bound array");
  @@ -1322,7 +1317,14 @@ public:
         assert(CGM.getContext().hasSameUnqualifiedType(Ty, Arg->getType()) &&
                "argument to copy ctor is of wrong type");
  
  -      return Visit(Arg, Ty);
  +      if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Arg)) {
  +        // Look through the temporary; it's just converting the value to an
  +        // lvalue to pass it to the constructor.
  +        return Visit(MTE->getSubExpr(), Ty);
  +      }
  +
  +      // Don't try to support arbitrary lvalue-to-rvalue conversions for now.
  +      return nullptr;
       }
  
       return CGM.EmitNullConstant(Ty);



- HasAnyMaterializeTemporaryExpr shouldn't need to handle VisitCXXConstructExpr.  The EvaluateAsLValue bug only applies to lifetime-extended temporaries.  Any MaterializeTemporaryExpr that's the operand of a VisitCXXConstructExpr should not be lifetime-extended beyond the end of the expression. So it won't run into the EvaluateAsLValue issue.  (https://en.cppreference.com/w/cpp/language/reference_initialization#Lifetime_of_a_temporary should be a complete list of cases where we might need to recurse; none of those corresponds to a CXXConstructExpr.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587