[clang] WIP: Implement simpler alternative to two-phase lookup for NRVO

[mizvekov]

Summary

WIP, not ready for review.

Signed-off-by: Matheus Izvekov <[e-mail]>

[mizvekov]

.

[mizvekov]

.

[mizvekov]

Initial implementation, still WIP.

[mizvekov]

.

[mizvekov]

Patch now in workable state. Is Missing a few cleanups in the implementation.

Updates coroutine implicit move tests, as now they can finally match C++20 behavior and don't need P2266 retroactively applied.

There are some test result changes.
These all match current GCC behaviour though: We are now broken in the same way as GCC is for these.
TBD what to do about these. The ambiguous overload resolution one might be easy to solve,
the other ones have to be studied more carefully.

clang/include/clang/Sema/Sema.h

public:
 
   struct NRVOResult {
     const VarDecl *Candidate;
 
     enum Status : int8_t { None, MoveEligible, MoveEligibleAndCopyElidable };
     Status S;
     bool isParenthesized;
 
-    bool isMoveEligible() const { return S >= MoveEligible; };
     bool isCopyElidable() const { return S == MoveEligibleAndCopyElidable; }
   };
-  NRVOResult getNRVOResult(Expr *&E, bool ForceCXX2b = false);
-  NRVOResult getNRVOResult(const VarDecl *VD, bool Parenthesized = false,
-                           bool ForceCXX20 = false);
+  NRVOResult getNRVOResult(Expr *&E);
+  NRVOResult getNRVOResult(const VarDecl *VD, bool Parenthesized = false);
   void updNRVOResultWithRetType(NRVOResult &Res, QualType ReturnType);
 
-  ExprResult PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
-                                             const NRVOResult &NRVORes,
-                                             Expr *Value);
-
   StmtResult ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
                              Scope *CurScope);
   StmtResult BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp);
   StmtResult ActOnCapScopeReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
                                      NRVOResult &NRVORes);
 
   StmtResult ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
                              bool IsVolatile, unsigned NumOutputs,

clang/lib/Sema/SemaCoroutine.cpp

if (E && E->getType()->isPlaceholderType() &&
     ExprResult R = CheckPlaceholderExpr(E);
     if (R.isInvalid()) return StmtError();
     E = R.get();
   }
 
   VarDecl *Promise = FSI->CoroutinePromise;
   ExprResult PC;
   if (E && (isa<InitListExpr>(E) || !E->getType()->isVoidType())) {
-    getNRVOResult(E, /*ForceCXX2b=*/true);
+    getNRVOResult(E);
     PC = buildPromiseCall(*this, Promise, Loc, "return_value", E);
   } else {
     E = MakeFullDiscardedValueExpr(E).get();
     PC = buildPromiseCall(*this, Promise, Loc, "return_void", None);
   }
   if (PC.isInvalid())
     return StmtError();
 

clang/lib/Sema/SemaExprCXX.cpp

if (Ex && !Ex->isTypeDependent()) {
     //       innermost enclosing try-block (if there is one), the copy/move
     //       operation from the operand to the exception object (15.1) can be
     //       omitted by constructing the automatic object directly into the
     //       exception object
 
     InitializedEntity Entity = InitializedEntity::InitializeException(
         OpLoc, ExceptionObjectTy,
         /*NRVO=*/NRVORes.isCopyElidable());
-    ExprResult Res = PerformMoveOrCopyInitialization(Entity, NRVORes, Ex);
+    ExprResult Res = PerformCopyInitialization(Entity, SourceLocation(), 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())
     CheckPPCMMAType(Ex->getType(), Ex->getBeginLoc());

clang/lib/Sema/SemaStmt.cpp

 /// from was parenthesized.
 ///
 /// \param ForceCXX20 Overrides detection of current language mode
 /// and uses the rules for C++20.
 ///
 /// \returns An aggregate which contains the Candidate and isMoveEligible
 /// and isCopyElidable methods. If Candidate is non-null, it means
 /// isMoveEligible() would be true under the most permissive language standard.
 Sema::NRVOResult Sema::getNRVOResult(const VarDecl *VD, bool Parenthesized,
                                      bool ForceCXX20) {
   if (!VD)
     return NRVOResult();
 
   bool hasCXX11 = getLangOpts().CPlusPlus11 || ForceCXX20,
        hasCXX20 = getLangOpts().CPlusPlus20 || ForceCXX20;
   NRVOResult Res{VD, NRVOResult::MoveEligibleAndCopyElidable, Parenthesized};
 

 /// might be modified by the implementation.
 ///
 /// \param ForceCXX2b Overrides detection of current language mode
 /// and uses the rules for C++2b.
 ///
 /// \returns An aggregate which contains the Candidate and isMoveEligible
 /// and isCopyElidable methods. If Candidate is non-null, it means
 /// isMoveEligible() would be true under the most permissive language standard.
-Sema::NRVOResult Sema::getNRVOResult(Expr *&E, bool ForceCXX2b) {
+Sema::NRVOResult Sema::getNRVOResult(Expr *&E) {
   if (!E)
     return NRVOResult();
   bool Parenthesized = isa<ParenExpr>(E);
   // - in a return statement in a function [where] ...
   // ... the expression is the name of a non-volatile automatic object ...
   const auto *DR = dyn_cast<DeclRefExpr>(E->IgnoreParens());
   if (!DR || DR->refersToEnclosingVariableOrCapture())
     return NRVOResult();
   const auto *VD = dyn_cast<VarDecl>(DR->getDecl());
-  NRVOResult Res = getNRVOResult(VD, Parenthesized, /*ForceCXX20=*/ForceCXX2b);
-  if (Res.Candidate && E->getValueKind() != VK_XValue &&
-      (ForceCXX2b || getLangOpts().CPlusPlus2b)) {
+  NRVOResult Res = getNRVOResult(VD, Parenthesized);
+  if (Res.S >= NRVOResult::MoveEligible) {
+    ExprValueKind CastToKind = VK_XValue;
+    if (E->getValueKind() != CastToKind) {
       E = ImplicitCastExpr::Create(Context, VD->getType().getNonReferenceType(),
-                                 CK_NoOp, E, nullptr, VK_XValue,
+                                   CK_NoOp, E, nullptr, CastToKind,
                                    FPOptionsOverride());
     }
+  } else if (Res.Candidate &&
+             !getDiagnostics().isIgnored(diag::warn_return_std_move,
+                                         E->getExprLoc())) {
+    QualType QT = Res.Candidate->getType();
+    if (QT->isLValueReferenceType()) {
+      // Adding 'std::move' around an lvalue reference variable's name is
+      // dangerous. Don't suggest it.
+    } else if (QT.getNonReferenceType()
+                   .getUnqualifiedType()
+                   .isTriviallyCopyableType(Context)) {
+      // Adding 'std::move' around a trivially copyable variable is probably
+      // pointless. Don't suggest it.
+    } else {
+      bool IsThrow =
+          false; //(Entity.getKind() == InitializedEntity::EK_Exception);
+      SmallString<32> Str;
+      Str += "std::move(";
+      Str += Res.Candidate->getDeclName().getAsString();
+      Str += ")";
+      Diag(E->getExprLoc(), diag::warn_return_std_move)
+          << E->getSourceRange() << Res.Candidate->getDeclName() << IsThrow;
+      Diag(E->getExprLoc(), diag::note_add_std_move)
+          << FixItHint::CreateReplacement(E->getSourceRange(), Str);
+    }
+  }
   return Res;
 }
 
 /// Updates given NRVOResult's move-eligible and
 /// copy-elidable statuses, considering the function return type criteria
 /// as applicable to return statements.
 ///
 /// \param Res The NRVOResult object to update.
void Sema::updNRVOResultWithRetType(NRVOResult &Res, QualType ReturnType) {
 
   // ... the same cv-unqualified type as the function return type ...
   // When considering moving this expression out, allow dissimilar types.
   if (!VDType->isDependentType() &&
       !Context.hasSameUnqualifiedType(ReturnType, VDType))
     downgradeNRVOResult(Res, getLangOpts().CPlusPlus11);
 }
 
-/// Try to perform the initialization of a potentially-movable value,
-/// which is the operand to a return or throw statement.
-///
-/// This routine implements C++20 [class.copy.elision]p3, which attempts to
-/// treat returned lvalues as rvalues in certain cases (to prefer move
-/// construction), then falls back to treating them as lvalues if that failed.
-///
-/// \param ConvertingConstructorsOnly If true, follow [class.copy.elision]p3 and
-/// reject resolutions that find non-constructors, such as derived-to-base
-/// conversions or `operator T()&&` member functions. If false, do consider such
-/// conversion sequences.
-///
-/// \param Res We will fill this in if move-initialization was possible.
-/// If move-initialization is not possible, such that we must fall back to
-/// treating the operand as an lvalue, we will leave Res in its original
-/// invalid state.
-///
-/// \returns Whether we need to do the second overload resolution. If the first
-/// overload resolution fails, or if the first overload resolution succeeds but
-/// the selected constructor/operator doesn't match the additional criteria, we
-/// need to do the second overload resolution.
-static bool TryMoveInitialization(Sema &S, const InitializedEntity &Entity,
-                                  const VarDecl *NRVOCandidate, Expr *&Value,
-                                  bool ConvertingConstructorsOnly,
-                                  bool IsDiagnosticsCheck, ExprResult &Res) {
-  ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
-                            CK_NoOp, Value, VK_XValue, FPOptionsOverride());
-
-  Expr *InitExpr = &AsRvalue;
-
-  InitializationKind Kind = InitializationKind::CreateCopy(
-      Value->getBeginLoc(), Value->getBeginLoc());
-
-  InitializationSequence Seq(S, Entity, Kind, InitExpr);
-
-  bool NeedSecondOverloadResolution = true;
-  if (!Seq &&
-      (IsDiagnosticsCheck || Seq.getFailedOverloadResult() != OR_Deleted)) {
-    return NeedSecondOverloadResolution;
-  }
-
-  for (const InitializationSequence::Step &Step : Seq.steps()) {
-    if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
-        Step.Kind != InitializationSequence::SK_UserConversion)
-      continue;
-
-    FunctionDecl *FD = Step.Function.Function;
-    if (ConvertingConstructorsOnly) {
-      if (isa<CXXConstructorDecl>(FD)) {
-        // C++11 [class.copy]p32:
-        // C++14 [class.copy]p32:
-        // C++17 [class.copy.elision]p3:
-        // [...] if the type of the first parameter of the selected constructor
-        // is not an rvalue reference to the object's type (possibly
-        // cv-qualified), overload resolution is performed again, considering
-        // the object as an lvalue.
-        const RValueReferenceType *RRefType =
-            FD->getParamDecl(0)->getType()->getAs<RValueReferenceType>();
-        if (!RRefType)
-          break;
-        if (!S.Context.hasSameUnqualifiedType(RRefType->getPointeeType(),
-                                              NRVOCandidate->getType()))
-          break;
-      } else {
-        continue;
-      }
-    } else {
-      if (isa<CXXConstructorDecl>(FD)) {
-        // Check that overload resolution selected a constructor taking an
-        // rvalue reference. If it selected an lvalue reference, then we
-        // didn't need to cast this thing to an rvalue in the first place.
-        if (IsDiagnosticsCheck &&
-            !isa<RValueReferenceType>(FD->getParamDecl(0)->getType()))
-          break;
-      } else if (isa<CXXMethodDecl>(FD)) {
-        // Check that overload resolution selected a conversion operator
-        // taking an rvalue reference.
-        if (cast<CXXMethodDecl>(FD)->getRefQualifier() != RQ_RValue)
-          break;
-      } else {
-        continue;
-      }
-    }
-
-    NeedSecondOverloadResolution = false;
-    // Promote "AsRvalue" to the heap, since we now need this
-    // expression node to persist.
-    Value =
-        ImplicitCastExpr::Create(S.Context, Value->getType(), CK_NoOp, Value,
-                                 nullptr, VK_XValue, FPOptionsOverride());
-
-    // Complete type-checking the initialization of the return type
-    // using the constructor we found.
-    Res = Seq.Perform(S, Entity, Kind, Value);
-  }
-
-  return NeedSecondOverloadResolution;
-}
-
-/// Perform the initialization of a potentially-movable value, which
-/// is the result of return value.
-///
-/// This routine implements C++20 [class.copy.elision]p3, which attempts to
-/// treat returned lvalues as rvalues in certain cases (to prefer move
-/// construction), then falls back to treating them as lvalues if that failed.
-ExprResult
-Sema::PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
-                                      const NRVOResult &NRVORes, Expr *Value) {
-
-  if (NRVORes.Candidate && !getLangOpts().CPlusPlus2b) {
-    if (NRVORes.isMoveEligible()) {
-      ExprResult Res;
-      if (!TryMoveInitialization(*this, Entity, NRVORes.Candidate, Value,
-                                 !getLangOpts().CPlusPlus20, false, Res))
-        return Res;
-    }
-    if (!getDiagnostics().isIgnored(diag::warn_return_std_move,
-                                    Value->getExprLoc())) {
-      QualType QT = NRVORes.Candidate->getType();
-      if (QT->isLValueReferenceType()) {
-        // Adding 'std::move' around an lvalue reference variable's name is
-        // dangerous. Don't suggest it.
-      } else if (QT.getNonReferenceType()
-                     .getUnqualifiedType()
-                     .isTriviallyCopyableType(Context)) {
-        // Adding 'std::move' around a trivially copyable variable is probably
-        // pointless. Don't suggest it.
-      } else {
-        ExprResult FakeRes = ExprError();
-        Expr *FakeValue = Value;
-        TryMoveInitialization(*this, Entity, NRVORes.Candidate, FakeValue,
-                              false, true, FakeRes);
-        if (!FakeRes.isInvalid()) {
-          bool IsThrow = (Entity.getKind() == InitializedEntity::EK_Exception);
-          SmallString<32> Str;
-          Str += "std::move(";
-          Str += NRVORes.Candidate->getDeclName().getAsString();
-          Str += ")";
-          Diag(Value->getExprLoc(), diag::warn_return_std_move)
-              << Value->getSourceRange() << NRVORes.Candidate->getDeclName()
-              << IsThrow;
-          Diag(Value->getExprLoc(), diag::note_add_std_move)
-              << FixItHint::CreateReplacement(Value->getSourceRange(), Str);
-        }
-      }
-    }
-  }
-
-  // Either we didn't meet the criteria for treating an lvalue as an rvalue,
-  // above, or overload resolution failed. Either way, we need to try
-  // (again) now with the return value expression as written.
-  return PerformCopyInitialization(Entity, SourceLocation(), Value);
-}
-
 /// Determine whether the declared return type of the specified function
 /// contains 'auto'.
 static bool hasDeducedReturnType(FunctionDecl *FD) {
   const FunctionProtoType *FPT =
       FD->getTypeSourceInfo()->getType()->castAs<FunctionProtoType>();
   return FPT->getReturnType()->isUndeducedType();
 }
 
if (FnRetType->isDependentType()) {
     // 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, NRVORes.isCopyElidable());
     ExprResult Res =
-        PerformMoveOrCopyInitialization(Entity, NRVORes, RetValExp);
+        PerformCopyInitialization(Entity, SourceLocation(), RetValExp);
     if (Res.isInvalid()) {
       // FIXME: Cleanup temporaries here, anyway?
       return StmtError();
     }
     RetValExp = Res.get();
     CheckReturnValExpr(RetValExp, FnRetType, ReturnLoc);
   }
 
if (FnRetType->isVoidType()) {
 
     // 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, NRVORes.isCopyElidable());
       ExprResult Res =
-          PerformMoveOrCopyInitialization(Entity, NRVORes, RetValExp);
+          PerformCopyInitialization(Entity, SourceLocation(), RetValExp);
       if (Res.isInvalid()) {
         // FIXME: Clean up temporaries here anyway?
         return StmtError();
       }
       RetValExp = Res.getAs<Expr>();
 
       // If we have a related result type, we need to implicitly
       // convert back to the formal result type.  We can't pretend to