[clang] don't mark as Elidable CXXConstruct expressions used in NRVO

[mizvekov]

Summary

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.

[mizvekov]

.

[mizvekov]

.

[mizvekov]

.

[rsmith]

There might be some tooling out there that cares about CXXConstructExprs that might be elided due to NRVO being marked as elidable. But it's probably OK to lose that marking if nothing in-tree is relying on it, and use isElidable only for copy elision from rvalues and not for NRVO.

I would have expected that we'd also assert for a case like this (in C++14 and earlier):

struct b {};
struct a : b {
  a();
  a(a &);
  a(const b &);
};
a c() {
  if (0) { return a(); }

  return a();
}

... and that this patch wouldn't help there. But it looks like we don't mark the a(const b&) constructor call here as elidable, which seems like a bug, but might not be one worth fixing?

[mizvekov]

I can have a look on what is happening with that test case. At best if we figure out what is wrong, we could add some FIXMEs and circle back to it after we make this whole thing work again.

[mizvekov]

Add a couple more test cases where we don't perform copy-elision
on a returned prvalue in pre-C++17 modes, and the corresponding
FIXMEs in the source code.

[mizvekov]

typo

[mizvekov]

.

[mizvekov]

add another FIXME.

[dim]

I can at least confirm that both the original test case for bug 51862 (from https://github.com/Macaulay2/frobby ) and the reduced test case compile successfully with this patch added.

[• Quuxplusone]

FWIW, this is almost entirely above my pay grade. Getting rid of that extra bool parameter throughout looks awesome, though. :)

I do think it might be a good idea to include one or two tests that actually run all the way through codegen, e.g. https://godbolt.org/z/54eTrj54M
The scary failure mode here (if you are like me and don't really understand why this is impossible ;)) would be if the compiler somehow elided one of the implicit conversions without eliding the other one (like how compiler bugs have in the past led to eliding a constructor without eliding the matching destructor).

Also, is it worth testing anything in constexpr functions? My understanding is that constexpr functions never do copy elision, although maybe(?) they're permitted to?

[mizvekov]

In D109800#3004288, @Quuxplusone wrote:

Getting rid of that extra bool parameter throughout looks awesome, though. :)

It will come back later, this patch is just a quick thing we need in order to ship clang-13.
But perhaps we bring it back in enum class form? :)

I do think it might be a good idea to include one or two tests that actually run all the way through codegen, e.g. https://godbolt.org/z/54eTrj54M

I think in that case, if we pursue that direction, what we would want is to have these tests that only go until LLVM IR, and then another set of tests that go from IR to arch specific LL, or perhaps just to optimized IR.
To be frank, I don't see how anything interesting here would happen after clang CodeGen (except in UB cases), but I am not very familiar with that side of the project.

The scary failure mode here (if you are like me and don't really understand why this is impossible ;)) would be if the compiler somehow elided one of the implicit conversions without eliding the other one (like how compiler bugs have in the past led to eliding a constructor without eliding the matching destructor).

Right, when we get to that point where we can elide that double conversion sequence, we got to add tests, but I suspect they don't need to go further than clang CodeGen.

Also, is it worth testing anything in constexpr functions? My understanding is that constexpr functions never do copy elision, although maybe(?) they're permitted to?

We do have analogous situation to CodeGen there, we step over a MaterializeTemporary in the isElidable case, and if we improve in this area to support double conversions, that has to be fixed too.
But this patch should not be changing anything in the rvalue path, only the NRVO one, which again I could not find anything that uses it anywhere in LLVM tree.
I will add an assert and FIXME there though.

[mizvekov]

add a couple more FIXMEs and a couple of asserts in the constexpr path for rvalue copy elision.

[dim]

clang/lib/AST/ExprConstant.cpp

9937

s/simples/simplest/

clang/lib/CodeGen/CGExprCXX.cpp

613–623

s/simples/simplest/

[mizvekov]

typo.

clang/include/clang/Sema/Initialization.h

InitializeTemplateParameter(QualType T, NonTypeTemplateParmDecl *Param) {
     Entity.Type = T;
     Entity.Parent = nullptr;
     Entity.Variable = {Param, false, false};
     return Entity;
   }
 
   /// 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,
                                             QualType Type) {
     return InitializedEntity(EK_StmtExprResult, ReturnLoc, Type);
   }
 
   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.
   static InitializedEntity InitializeNew(SourceLocation NewLoc, QualType Type) {
     return InitializedEntity(EK_New, NewLoc, Type);
   }
 
   /// Create the initialization entity for a temporary.

clang/lib/AST/ExprConstant.cpp

bool RecordExprEvaluator::VisitCXXConstructExpr(const CXXConstructExpr *E,
 
   const FunctionDecl *Definition = nullptr;
   auto Body = FD->getBody(Definition);
 
   if (!CheckConstexprFunction(Info, E->getExprLoc(), FD, Definition, Body))
     return false;
 
   // Avoid materializing a temporary for an elidable copy/move constructor.
-  if (E->isElidable() && !ZeroInit)
-    if (const MaterializeTemporaryExpr *ME
-          = dyn_cast<MaterializeTemporaryExpr>(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<MaterializeTemporaryExpr>(SrcObj))
       return Visit(ME->getSubExpr());
+  }
 
   if (ZeroInit && !ZeroInitialization(E, T))
     return false;
 
   auto Args = llvm::makeArrayRef(E->getArgs(), E->getNumArgs());
   return HandleConstructorCall(E, This, Args,
                                cast<CXXConstructorDecl>(Definition), Info,
                                Result);

clang/lib/CodeGen/CGExprCXX.cpp

if (E->requiresZeroInitialization() && !Dest.isZeroed()) {
     }
   }
 
   // If this is a call to a trivial default constructor, do nothing.
   if (CD->isTrivial() && CD->isDefaultConstructor())
     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);
 
   if (const ArrayType *arrayType
         = getContext().getAsArrayType(E->getType())) {
     EmitCXXAggrConstructorCall(CD, arrayType, Dest.getAddress(), E,
                                Dest.isSanitizerChecked());
   } else {
     CXXCtorType Type = Ctor_Complete;
     bool ForVirtualBase = false;

clang/lib/Sema/Sema.cpp

 static void checkEscapingByref(VarDecl *VD, Sema &S) {
   QualType T = VD->getType();
   EnterExpressionEvaluationContext scope(
       S, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
   SourceLocation Loc = VD->getLocation();
   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());
     Result = S.PerformCopyInitialization(IE, SourceLocation(), E);
   } else {
     Result = S.PerformMoveOrCopyInitialization(
         IE, Sema::NamedReturnInfo{VD, Sema::NamedReturnInfo::MoveEligible},
         VarRef);

clang/lib/Sema/SemaCoroutine.cpp

if (FnRetType->isVoidType()) {
 
     this->ResultDecl = Res.get();
     return true;
   }
 
   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;
   }
 
   auto *GroDecl = VarDecl::Create(
       S.Context, &FD, FD.getLocation(), FD.getLocation(),
       &S.PP.getIdentifierTable().get("__coro_gro"), GroType,

clang/lib/Sema/SemaDeclCXX.cpp

Sema::BuildCXXConstructExpr(SourceLocation ConstructLoc, QualType DeclInitType,
   //   copy/move constructor and/or destructor for the object have
   //   side effects. [...]
   //     - when a temporary class object that has not been bound to a
   //       reference (12.2) would be copied/moved to a class object
   //       with the same cv-unqualified type, the copy/move operation
   //       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<CXXRecordDecl>(FoundDecl->getDeclContext()));
   }
 
   return BuildCXXConstructExpr(ConstructLoc, DeclInitType,
                                FoundDecl, Constructor,
                                Elidable, ExprArgs, HadMultipleCandidates,
                                IsListInitialization,

clang/lib/Sema/SemaExpr.cpp

if (getLangOpts().CPlusPlus && Cap.isCopyCapture()) {
           Result = ImpCastExprToType(Result.get(),
                                      Result.get()->getType().withConst(),
                                      CK_NoOp, VK_LValue);
         }
 
         if (!Result.isInvalid()) {
           Result = PerformCopyInitialization(
               InitializedEntity::InitializeBlock(Var->getLocation(),
-                                                 Cap.getCaptureType(), false),
+                                                 Cap.getCaptureType()),
               Loc, Result.get());
         }
 
         // Build a full-expression copy expression if initialization
         // succeeded and used a non-trivial constructor.  Recover from
         // errors by pretending that the copy isn't necessary.
         if (!Result.isInvalid() &&
             !cast<CXXConstructExpr>(Result.get())->getConstructor()

clang/lib/Sema/SemaExprCXX.cpp

if (Ex && !Ex->isTypeDependent()) {
     //       exception object
     NamedReturnInfo NRInfo =
         IsThrownVarInScope ? getNamedReturnInfo(Ex) : NamedReturnInfo();
 
     QualType ExceptionObjectTy = Context.getExceptionObjectType(Ex->getType());
     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();
     Ex = Res.get();
   }
 
   // PPC MMA non-pointer types are not allowed as throw expr types.
   if (Ex && Context.getTargetInfo().getTriple().isPPC64())

clang/lib/Sema/SemaLambda.cpp

ExprResult Sema::BuildBlockForLambdaConversion(SourceLocation CurrentLocation,
   CXXMethodDecl *CallOperator
     = cast<CXXMethodDecl>(
         Lambda->lookup(
           Context.DeclarationNames.getCXXOperatorName(OO_Call)).front());
   CallOperator->setReferenced();
   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);
 
   if (Init.isInvalid())
     return ExprError();
 
   // Create the new block to be returned.

clang/lib/Sema/SemaObjCProperty.cpp

if (getLangOpts().CPlusPlus && Synthesize && !CompleteTypeErr &&
       Expr *IvarRefExpr =
         new (Context) ObjCIvarRefExpr(Ivar,
                                       Ivar->getUsageType(SelfDecl->getType()),
                                       PropertyDiagLoc,
                                       Ivar->getLocation(),
                                       LoadSelfExpr, true, true);
       ExprResult Res = PerformCopyInitialization(
           InitializedEntity::InitializeResult(PropertyDiagLoc,
-                                              getterMethod->getReturnType(),
-                                              /*NRVO=*/false),
+                                              getterMethod->getReturnType()),
           PropertyDiagLoc, IvarRefExpr);
       if (!Res.isInvalid()) {
         Expr *ResExpr = Res.getAs<Expr>();
         if (ResExpr)
           ResExpr = MaybeCreateExprWithCleanups(ResExpr);
         PIDecl->setGetterCXXConstructor(ResExpr);
       }
     }

clang/lib/Sema/SemaStmt.cpp

if (FnRetType->isDependentType()) {
     // we have a non-void block with an expression, continue checking
 
     // C99 6.8.6.4p3(136): The return statement is not an assignment. The
     // overlap restriction of subclause 6.5.16.1 does not apply to the case of
     // function return.
 
     // 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()) {
       // FIXME: Cleanup temporaries here, anyway?
       return StmtError();
     }
     RetValExp = Res.get();
     CheckReturnValExpr(RetValExp, FnRetType, ReturnLoc);
if (FnRetType->isVoidType()) {
     // C99 6.8.6.4p3(136): The return statement is not an assignment. The
     // overlap restriction of subclause 6.5.16.1 does not apply to the case of
     // function return.
 
     // In C++ the return statement is handled via a copy initialization,
     // 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()) {
         // FIXME: Clean up temporaries here anyway?
         return StmtError();
       }
       RetValExp = Res.getAs<Expr>();
 

clang/test/CodeGen/nrvo-tracking.cpp

 // CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
 // CHECK-NEXT:  call void @llvm.lifetime.end
 X t5() {
   X a [[gnu::aligned(1)]] [[gnu::aligned(8)]];
   return a;
 }
 
 } // namespace test_alignas
+
+namespace PR51862 {
+
+template <class T> 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<A>();
+
+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<B>();
+
+} // namespace PR51862

clang/test/CodeGenCXX/copy-elision.cpp

+// RUN: %clang_cc1 -std=c++14 -triple x86_64-unknown-unknown-gnu -emit-llvm -O1 -fexperimental-new-pass-manager -o - %s | FileCheck %s
+
+template <class T> 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<A>();
+
+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<B>();