[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:4728
 
-  enum CopyElisionSemanticsKind {
-    CES_Strict = 0,
-    CES_AllowParameters = 1,
-    CES_AllowDifferentTypes = 2,
-    CES_AllowExceptionVariables = 4,
-    CES_AllowRValueReferenceType = 8,
-    CES_ImplicitlyMovableCXX11CXX14CXX17 =
-        (CES_AllowParameters | CES_AllowDifferentTypes),
-    CES_ImplicitlyMovableCXX20 =
-        (CES_AllowParameters | CES_AllowDifferentTypes |
-         CES_AllowExceptionVariables | CES_AllowRValueReferenceType),
+  struct NRVOResult {
+    const VarDecl *Candidate;
----------------
We generally use `FooResult` to mean "either a `Foo` or an indicator that an error has been diagnosed". Would the name `NRVOInfo` work instead?

Actually, I think the "O" here is misleading, because we're not talking about performing the optimization here, just about what variable was named in a return statement. And I think calling this "NRVO" is a bit misleading because it also covers implicit move, which isn't traditionally part of NRVO. So how about `NamedReturnValueInfo` or `NamedReturnInfo` or `NamedReturnValue` or `ReturnValueName` or something like that?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3066
 
-  if (isCopyElisionCandidate(ReturnType, VD, CESK))
-    return VD;
-  return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult &Res, bool CanMove) {
+  Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible
----------------
I find this name a little unclear (what do we mean by "downgrade"?). Can we call this something a bit more specific, such as `disallowCopyElision` or `disallowNRVO`?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3138
+/// copy-elidable statuses, considering the function return type criteria
+/// as applicable to return statements.
+///
----------------
This comment needs re-flowing.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3144
+/// \returns The copy elision candidate, in case the initial return expression
+/// was copy elisible, or nullptr otherwise.
+const VarDecl *Sema::getCopyElisionCandidate(NRVOResult &Res,
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:3150-3153
+  // If we got a non-deduced auto ReturnType, we are in a dependent context and
+  // there is no point in allowing copy elision since we won't have it deduced
+  // by the point the VardDecl is instantiated, which is the last chance we have
+  // of deciding if the candidate is really copy elisible.
----------------
How does this happen? Are there any cases where we could do NRVO or should do an implicit move that are blocked by this?

It seems to me that we should (nearly always) be able to work out whether copy elision is possible here without knowing the deduced type:
-- if the return type is //cv// `auto` then it will always be deduced to the type of the returned variable, so we can always perform copy elision
-- if the return type is `decltype(auto)`, then we can perform copy elision if the expression is unparenthesized and otherwise cannot; we could perhaps track whether the expression was parenthesized in `NRVOResult`, and can conservatively disallow copy elision if we don't know (eg, from template instantiation, where we're only looking at the variable and not the return statements)
-- if the return type is anything else involving `auto`, it can't possibly instantiate to a class type, so we'll never perform copy elision


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3152-3153
+  // there is no point in allowing copy elision since we won't have it deduced
+  // by the point the VardDecl is instantiated, which is the last chance we have
+  // of deciding if the candidate is really copy elisible.
+  if (AutoType *AT = ReturnType->getContainedAutoType())
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:3156
+    if (AT->isCanonicalUnqualified())
+      return Res = NRVOResult(), nullptr;
+
----------------
Please add braces rather than using a comma expression here and below.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3290
     }
-
-    if (!getLangOpts().CPlusPlus20 && NeedSecondOverloadResolution &&
-        !getDiagnostics().isIgnored(diag::warn_return_std_move,
+    if (!getDiagnostics().isIgnored(diag::warn_return_std_move,
                                     Value->getExprLoc())) {
----------------
Should we skip this if `NRVORes.isMoveEligible() && getLangOpts().CPlusPlus20`? In that case, we already tried an unrestricted move a couple of lines ago, so we know that adding `std::move` won't help. (I think we can still hit the "adding `std::move` will help" case in C++20, but only in obscure corners such as a `volatile`-qualified local variable.)


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3470
+    InitializedEntity Entity = InitializedEntity::InitializeResult(
+        ReturnLoc, FnRetType, !!NRVOCandidate);
+    ExprResult Res =
----------------
I find the pre-existing `NRVOCandidate != nullptr` more readable than this `!!NRVOCandidate`.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1058
+    else if (auto *F = dyn_cast<BlockDecl>(DC))
+      // FIXME: get the return type here somehow...
+      ReturnType = SemaRef.Context.getAutoDeductType();
----------------
Can you use `getCurBlock()->FunctionType` for this? We won't have a `Scope`, but we should still have a `ScopeInfo`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696