From d9308aa39b236064a680ca57178af3c731e13e49 Mon Sep 17 00:00:00 2001 From: Matheus Izvekov Date: Wed, 15 Sep 2021 01:46:30 +0200 Subject: [PATCH] [clang] don't mark as Elidable CXXConstruct expressions used in NRVO See PR51862. The consumers of the Elidable flag in CXXConstructExpr assume that an elidable construction just goes through a single copy/move construction, so that the source object is immediately passed as an argument and is the same type as the parameter itself. With the implementation of P2266 and after some adjustments to the implementation of P1825, we started (correctly, as per standard) allowing more cases where the copy initialization goes through user defined conversions. With this patch we stop using this flag in NRVO contexts, to preserve code that relies on that assumption. This causes no known functional changes, we just stop firing some asserts in a cople of included test cases. Reviewed By: rsmith Differential Revision: https://reviews.llvm.org/D109800 --- clang/include/clang/Sema/Initialization.h | 16 +++++----- clang/lib/AST/ExprConstant.cpp | 15 +++++++-- clang/lib/CodeGen/CGExprCXX.cpp | 19 +++++++----- clang/lib/Sema/Sema.cpp | 2 +- clang/lib/Sema/SemaCoroutine.cpp | 2 +- clang/lib/Sema/SemaDeclCXX.cpp | 9 ++++++ clang/lib/Sema/SemaExpr.cpp | 2 +- clang/lib/Sema/SemaExprCXX.cpp | 5 ++- clang/lib/Sema/SemaLambda.cpp | 3 +- clang/lib/Sema/SemaObjCProperty.cpp | 3 +- clang/lib/Sema/SemaStmt.cpp | 8 ++--- clang/test/CodeGen/nrvo-tracking.cpp | 37 +++++++++++++++++++++++ clang/test/CodeGenCXX/copy-elision.cpp | 34 +++++++++++++++++++++ 13 files changed, 122 insertions(+), 33 deletions(-) create mode 100644 clang/test/CodeGenCXX/copy-elision.cpp diff --git a/clang/include/clang/Sema/Initialization.h b/clang/include/clang/Sema/Initialization.h index 4664861c4e32a6..8c1856f2082796 100644 --- a/clang/include/clang/Sema/Initialization.h +++ b/clang/include/clang/Sema/Initialization.h @@ -298,8 +298,8 @@ class alignas(8) InitializedEntity { /// Create the initialization entity for the result of a function. static InitializedEntity InitializeResult(SourceLocation ReturnLoc, - QualType Type, bool NRVO) { - return InitializedEntity(EK_Result, ReturnLoc, Type, NRVO); + QualType Type) { + return InitializedEntity(EK_Result, ReturnLoc, Type); } static InitializedEntity InitializeStmtExprResult(SourceLocation ReturnLoc, @@ -308,20 +308,20 @@ class alignas(8) InitializedEntity { } static InitializedEntity InitializeBlock(SourceLocation BlockVarLoc, - QualType Type, bool NRVO) { - return InitializedEntity(EK_BlockElement, BlockVarLoc, Type, NRVO); + QualType Type) { + return InitializedEntity(EK_BlockElement, BlockVarLoc, Type); } static InitializedEntity InitializeLambdaToBlock(SourceLocation BlockVarLoc, - QualType Type, bool NRVO) { + QualType Type) { return InitializedEntity(EK_LambdaToBlockConversionBlockElement, - BlockVarLoc, Type, NRVO); + BlockVarLoc, Type); } /// Create the initialization entity for an exception object. static InitializedEntity InitializeException(SourceLocation ThrowLoc, - QualType Type, bool NRVO) { - return InitializedEntity(EK_Exception, ThrowLoc, Type, NRVO); + QualType Type) { + return InitializedEntity(EK_Exception, ThrowLoc, Type); } /// Create the initialization entity for an object allocated via new. diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp index 296e9e7e5995e6..ea7818c43e3213 100644 --- a/clang/lib/AST/ExprConstant.cpp +++ b/clang/lib/AST/ExprConstant.cpp @@ -9933,10 +9933,19 @@ bool RecordExprEvaluator::VisitCXXConstructExpr(const CXXConstructExpr *E, return false; // Avoid materializing a temporary for an elidable copy/move constructor. - if (E->isElidable() && !ZeroInit) - if (const MaterializeTemporaryExpr *ME - = dyn_cast(E->getArg(0))) + if (E->isElidable() && !ZeroInit) { + // FIXME: This only handles the simplest case, where the source object + // is passed directly as the first argument to the constructor. + // This should also handle stepping though implicit casts and + // and conversion sequences which involve two steps, with a + // conversion operator followed by a converting constructor. + const Expr *SrcObj = E->getArg(0); + assert(SrcObj->isTemporaryObject(Info.Ctx, FD->getParent())); + assert(Info.Ctx.hasSameUnqualifiedType(E->getType(), SrcObj->getType())); + if (const MaterializeTemporaryExpr *ME = + dyn_cast(SrcObj)) return Visit(ME->getSubExpr()); + } if (ZeroInit && !ZeroInitialization(E, T)) return false; diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp index d86ae7ad17d680..cc838bf38c6cad 100644 --- a/clang/lib/CodeGen/CGExprCXX.cpp +++ b/clang/lib/CodeGen/CGExprCXX.cpp @@ -609,15 +609,18 @@ CodeGenFunction::EmitCXXConstructExpr(const CXXConstructExpr *E, return; // Elide the constructor if we're constructing from a temporary. - // The temporary check is required because Sema sets this on NRVO - // returns. if (getLangOpts().ElideConstructors && E->isElidable()) { - assert(getContext().hasSameUnqualifiedType(E->getType(), - E->getArg(0)->getType())); - if (E->getArg(0)->isTemporaryObject(getContext(), CD->getParent())) { - EmitAggExpr(E->getArg(0), Dest); - return; - } + // FIXME: This only handles the simplest case, where the source object + // is passed directly as the first argument to the constructor. + // This should also handle stepping though implicit casts and + // conversion sequences which involve two steps, with a + // conversion operator followed by a converting constructor. + const Expr *SrcObj = E->getArg(0); + assert(SrcObj->isTemporaryObject(getContext(), CD->getParent())); + assert( + getContext().hasSameUnqualifiedType(E->getType(), SrcObj->getType())); + EmitAggExpr(SrcObj, Dest); + return; } if (const ArrayType *arrayType diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp index c997c70d17cef7..d260a45867e06b 100644 --- a/clang/lib/Sema/Sema.cpp +++ b/clang/lib/Sema/Sema.cpp @@ -2022,7 +2022,7 @@ static void checkEscapingByref(VarDecl *VD, Sema &S) { Expr *VarRef = new (S.Context) DeclRefExpr(S.Context, VD, false, T, VK_LValue, Loc); ExprResult Result; - auto IE = InitializedEntity::InitializeBlock(Loc, T, false); + auto IE = InitializedEntity::InitializeBlock(Loc, T); if (S.getLangOpts().CPlusPlus2b) { auto *E = ImplicitCastExpr::Create(S.Context, T, CK_NoOp, VarRef, nullptr, VK_XValue, FPOptionsOverride()); diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp index 94c728093e7c9f..3d1899a57c72a0 100644 --- a/clang/lib/Sema/SemaCoroutine.cpp +++ b/clang/lib/Sema/SemaCoroutine.cpp @@ -1533,7 +1533,7 @@ bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() { if (GroType->isVoidType()) { // Trigger a nice error message. InitializedEntity Entity = - InitializedEntity::InitializeResult(Loc, FnRetType, false); + InitializedEntity::InitializeResult(Loc, FnRetType); S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue); noteMemberDeclaredHere(S, ReturnValue, Fn); return false; diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index c3fbcc42fbfd39..4c5e5549e805d9 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -15312,8 +15312,17 @@ Sema::BuildCXXConstructExpr(SourceLocation ConstructLoc, QualType DeclInitType, // can be omitted by constructing the temporary object // directly into the target of the omitted copy/move if (ConstructKind == CXXConstructExpr::CK_Complete && Constructor && + // FIXME: Converting constructors should also be accepted. + // But to fix this, the logic that digs down into a CXXConstructExpr + // to find the source object needs to handle it. + // Right now it assumes the source object is passed directly as the + // first argument. Constructor->isCopyOrMoveConstructor() && hasOneRealArgument(ExprArgs)) { Expr *SubExpr = ExprArgs[0]; + // FIXME: Per above, this is also incorrect if we want to accept + // converting constructors, as isTemporaryObject will + // reject temporaries with different type from the + // CXXRecord itself. Elidable = SubExpr->isTemporaryObject( Context, cast(FoundDecl->getDeclContext())); } diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 7991e4c5e8b0a2..8eee366fbec67c 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -15713,7 +15713,7 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc, if (!Result.isInvalid()) { Result = PerformCopyInitialization( InitializedEntity::InitializeBlock(Var->getLocation(), - Cap.getCaptureType(), false), + Cap.getCaptureType()), Loc, Result.get()); } diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp index e76c4509d7888b..f4a27a00a2e2ef 100644 --- a/clang/lib/Sema/SemaExprCXX.cpp +++ b/clang/lib/Sema/SemaExprCXX.cpp @@ -893,9 +893,8 @@ ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex, if (CheckCXXThrowOperand(OpLoc, ExceptionObjectTy, Ex)) return ExprError(); - InitializedEntity Entity = InitializedEntity::InitializeException( - OpLoc, ExceptionObjectTy, - /*NRVO=*/NRInfo.isCopyElidable()); + InitializedEntity Entity = + InitializedEntity::InitializeException(OpLoc, ExceptionObjectTy); ExprResult Res = PerformMoveOrCopyInitialization(Entity, NRInfo, Ex); if (Res.isInvalid()) return ExprError(); diff --git a/clang/lib/Sema/SemaLambda.cpp b/clang/lib/Sema/SemaLambda.cpp index 3784938847b053..286471997db789 100644 --- a/clang/lib/Sema/SemaLambda.cpp +++ b/clang/lib/Sema/SemaLambda.cpp @@ -1978,8 +1978,7 @@ ExprResult Sema::BuildBlockForLambdaConversion(SourceLocation CurrentLocation, CallOperator->markUsed(Context); ExprResult Init = PerformCopyInitialization( - InitializedEntity::InitializeLambdaToBlock(ConvLocation, Src->getType(), - /*NRVO=*/false), + InitializedEntity::InitializeLambdaToBlock(ConvLocation, Src->getType()), CurrentLocation, Src); if (!Init.isInvalid()) Init = ActOnFinishFullExpr(Init.get(), /*DiscardedValue*/ false); diff --git a/clang/lib/Sema/SemaObjCProperty.cpp b/clang/lib/Sema/SemaObjCProperty.cpp index a329d0f22b0303..74c73ace3c5ff7 100644 --- a/clang/lib/Sema/SemaObjCProperty.cpp +++ b/clang/lib/Sema/SemaObjCProperty.cpp @@ -1467,8 +1467,7 @@ Decl *Sema::ActOnPropertyImplDecl(Scope *S, LoadSelfExpr, true, true); ExprResult Res = PerformCopyInitialization( InitializedEntity::InitializeResult(PropertyDiagLoc, - getterMethod->getReturnType(), - /*NRVO=*/false), + getterMethod->getReturnType()), PropertyDiagLoc, IvarRefExpr); if (!Res.isInvalid()) { Expr *ResExpr = Res.getAs(); diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp index fae2b143219b20..98e6275bfda076 100644 --- a/clang/lib/Sema/SemaStmt.cpp +++ b/clang/lib/Sema/SemaStmt.cpp @@ -3653,8 +3653,8 @@ StmtResult Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc, // In C++ the return statement is handled via a copy initialization. // the C version of which boils down to CheckSingleAssignmentConstraints. - InitializedEntity Entity = InitializedEntity::InitializeResult( - ReturnLoc, FnRetType, NRVOCandidate != nullptr); + InitializedEntity Entity = + InitializedEntity::InitializeResult(ReturnLoc, FnRetType); ExprResult Res = PerformMoveOrCopyInitialization( Entity, NRInfo, RetValExp, SupressSimplerImplicitMoves); if (Res.isInvalid()) { @@ -4085,8 +4085,8 @@ StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) { // the C version of which boils down to CheckSingleAssignmentConstraints. if (!HasDependentReturnType && !RetValExp->isTypeDependent()) { // we have a non-void function with an expression, continue checking - InitializedEntity Entity = InitializedEntity::InitializeResult( - ReturnLoc, RetType, NRVOCandidate != nullptr); + InitializedEntity Entity = + InitializedEntity::InitializeResult(ReturnLoc, RetType); ExprResult Res = PerformMoveOrCopyInitialization( Entity, NRInfo, RetValExp, SupressSimplerImplicitMoves); if (Res.isInvalid()) { diff --git a/clang/test/CodeGen/nrvo-tracking.cpp b/clang/test/CodeGen/nrvo-tracking.cpp index 2d6eb9efeca207..be405878f6f9b6 100644 --- a/clang/test/CodeGen/nrvo-tracking.cpp +++ b/clang/test/CodeGen/nrvo-tracking.cpp @@ -282,3 +282,40 @@ X t5() { } } // namespace test_alignas + +namespace PR51862 { + +template T test() { + T a; + T b; + if (0) + return a; + return b; +} + +struct A { + A(); + A(A &); + A(int); + operator int(); +}; + +// CHECK-LABEL: define{{.*}} void @_ZN7PR518624testINS_1AEEET_v +// CHECK: call i32 @_ZN7PR518621AcviEv +// CHECK-NEXT: call void @_ZN7PR518621AC1Ei +// CHECK-NEXT: call void @llvm.lifetime.end +template A test(); + +struct BSub {}; +struct B : BSub { + B(); + B(B &); + B(const BSub &); +}; + +// CHECK-LABEL: define{{.*}} void @_ZN7PR518624testINS_1BEEET_v +// CHECK: call void @_ZN7PR518621BC1ERKNS_4BSubE +// CHECK-NEXT: call void @llvm.lifetime.end +template B test(); + +} // namespace PR51862 diff --git a/clang/test/CodeGenCXX/copy-elision.cpp b/clang/test/CodeGenCXX/copy-elision.cpp new file mode 100644 index 00000000000000..72b2a5e00264c9 --- /dev/null +++ b/clang/test/CodeGenCXX/copy-elision.cpp @@ -0,0 +1,34 @@ +// RUN: %clang_cc1 -std=c++14 -triple x86_64-unknown-unknown-gnu -emit-llvm -O1 -fexperimental-new-pass-manager -o - %s | FileCheck %s + +template T test() { + return T(); +} + +struct A { + A(); + A(A &); + A(int); + operator int(); +}; + +// FIXME: There should be copy elision here. +// CHECK-LABEL: define{{.*}} void @_Z4testI1AET_v +// CHECK: call void @_ZN1AC1Ev +// CHECK-NEXT: call i32 @_ZN1AcviEv +// CHECK-NEXT: call void @_ZN1AC1Ei +// CHECK-NEXT: call void @llvm.lifetime.end +template A test(); + +struct BSub {}; +struct B : BSub { + B(); + B(B &); + B(const BSub &); +}; + +// FIXME: There should be copy elision here. +// CHECK-LABEL: define{{.*}} void @_Z4testI1BET_v +// CHECK: call void @_ZN1BC1Ev +// CHECK: call void @_ZN1BC1ERK4BSub +// CHECK-NEXT: call void @llvm.lifetime.end +template B test();