[PATCH] D99005: [clang] Implement P2266 Simpler implicit move

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1059
+    else if (auto *F = dyn_cast<BlockDecl>(DC))
+      goto unimplemented; // FIXME: get the return type here somehow...
+    else
----------------
rjmccall wrote:
> mizvekov wrote:
> > rjmccall wrote:
> > > mizvekov wrote:
> > > > @rjmccall Hello! Do you have any tips for fixing this one?
> > > I've always found it annoying that we only store the block type on the `BlockExpr` and not the `BlockDecl`, so if you've got a good reason to change that, I'm not opposed.
> > > 
> > > That said, while I'm not an expert in how we do NRVO variable tracking, the code must have some ability to handle an unknown return type when just processing declarations by delaying some of the bookkeeping/analysis until the end of the function.  It's possible but quite uncommon to declare a return type explicitly in a block, so that's the dominant case here.  I'm actually surprised we do any of this tracking when we're just instantiating a variable.
> > Thanks, your observations have been extremely helpful.
> > 
> > So this whole NRVO tracking thing looks to be in bad shape. This tracker here fails to handle 'auto' types. I don't think this gets fixed up anywhere else.
> > 
> > This is also badly covered by the test suite. The test included with the merge request that introduced this tracking (`CodeGenCXX/nrvo.cpp`) still passes if you either delete this whole block here, or if you just do `Var->setNRVOVariable(D->isNRVOVariable())`.
> > 
> > In fact, almost the whole test suite passes with those changes, the only tests which fail are `CodeGenCXX/matrix-type-builtins.cpp` and `CodeGenCXX/matrix-type-operators.cpp` respectively.
> I'm surprised that this has any impact on matrix types; pinging @fhahn in case he understands what the interaction might be.
> 
> It looks like we're assuming that the parse-time analysis of NRVO candidates is correct except for possibly needing adjustment for dependent expressions / return types.  The parse-time analysis seems to work by keeping track of NRVO candidates on `Scope`; since we don't push meaningful `Scope`s during template instantiation, that won't work, so we have to trust the parse-time analysis.  But the parse-time analysis can also be overly conservative, mostly because of `if constexpr`; that might be enough of a corner case to not merit something better, though.  @rsmith is the expert here, though.
> 
> At any rate, it should be *correct* to handle non-function cases as if the return type were dependent, the same way it presumably works for functions with deduced return types.
The main thing here is that the NRVO criteria for function return type is ignored if the type is dependent.
But it is not ignored for a non-dependent auto type, "because `auto` is not a record type", so this evaluates as not a NRVO candidate.

It is strange that we make this distinction between dependent and non-dependent auto here.

This is actually the only case where this function is called where the FnRetType is possibly `auto`, so it's an understandable oversight if that is the case.

I see we don't have the type deduced here even for functions with a single return statement and no `if constexpr` or other indirection, where it seems it would have been possible to have just deduced it to some dependent type during parsing.

That said, my (pretty uninformed) guess here is that it should be safe to treat auto the same as dependent, and it is probably the most reasonable choice.
That we are (again my guess) just trying here to weed out some candidates to get better results in some simple greedy algorithm later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99005