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

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov 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;
----------------
rsmith wrote:
> 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?
I like `NamedReturnInfo`, will change.


================
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
----------------
rsmith wrote:
> 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`?
How about `demoteNRVOStatus`?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3156
+    if (AT->isCanonicalUnqualified())
+      return Res = NRVOResult(), nullptr;
+
----------------
rsmith wrote:
> Please add braces rather than using a comma expression here and below.
Using short utility lambda as an alternative to keep the main logic clear from clutter.


================
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())) {
----------------
rsmith wrote:
> 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.)
This goes to the documentation on `getNamedReturnInfo`,  return value, where it says that the Canidate member will be non-null in case implicit move would be permitted under the most permissive language standard.

So if Try

So if we are guarded under that condition, we would never 


================
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())) {
----------------
mizvekov wrote:
> rsmith wrote:
> > 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.)
> This goes to the documentation on `getNamedReturnInfo`,  return value, where it says that the Canidate member will be non-null in case implicit move would be permitted under the most permissive language standard.
> 
> So if Try
> 
> So if we are guarded under that condition, we would never 
Skipping it is just a small compilation performance optimization:

We are going to try that `TryMoveInitialization` again just below, this time with equivalent of `ForceCXX20 = true`. If we were already in c++20 mode, then that second call is going to be made with basically the same parameters and perform basically the same work and reach the same conclusion: Faliure, and the diagnostic won't be produced.

Now my take on why that looks confusing: To me `TryMoveInitialization` has a similar problem to the one I addressed in this patch for `getCopyElisionCandidate`, it relies on you calling it multiple times, once to try the the actual work, and if that fails, then you have to call it again, this time in 'permissive' more, to get the information you need for the diagnostics.

Now I did not try giving it the same treatment of returning an aggregate with all the information you need, so you can call it just once with one less parameter.
If you agree on that take, I'd be happy to try a similar improvement there as well.
If you don't think it's worth the noise, then it's all good, no worries :)

As a side note, I just noticed that the
`if (QT->isLValueReferenceType()) {` below is dead code, we are never going to consider this move eligible in any mode, and so Res.Candidate is going to be null.

We should not need to repeat here the checks we already do in getNRVOResult, so I am going to go ahead and remove it.

I think it may have been added because until just recently `getCopyElisionCandidate` was bugged and accepted LValue references, and I guess the problem was "fixed" in this diagnostic first.


================
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();
----------------
rsmith wrote:
> Can you use `getCurBlock()->FunctionType` for this? We won't have a `Scope`, but we should still have a `ScopeInfo`.
Thanks a lot! That works and now we get copy elision on dependent blocks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696