Update NRVO logic to support early return

[tzik]

Summary

The previous implementation misses an opportunity to apply NRVO (Named Return Value
Optimization) below. That discourages user to write early return code.

struct Foo {};

Foo f(bool b) {
  if (b)
    return Foo();
  Foo oo;
  return oo;
}

That is, we can/should apply RVO for a local variable if:

• It's directly returned by at least one return statement.
• And, all reachable return statements in its scope returns the variable directly.

While, the previous implementation disables the RVO in a scope if there are multiple return
statements that refers different variables.

On the new algorithm, local variables are in NRVO_Candidate state at first, and a return
statement changes it to NRVO_Disabled for all visible variables but the return statement refers.
Then, at the end of the function AST traversal, NRVO is enabled for variables in NRVO_Candidate
state and refers from at least one return statement.

[tzik]

Fix NRVO on template code, and fix a nested scope case

[tzik]

comment fix

[• Quuxplusone]

lib/Sema/SemaDecl.cpp

12757–12758

What is the purpose of this change?
If it's "no functional change" it should be done separately IMHO; if it is supposed to change codegen, then it needs some codegen tests. (From looking at the code: maybe this affects codegen on functions that return member function pointers by value?)

test/CodeGenCXX/nrvo.cpp

139

You've changed this function from testing one thing with a FIXME, to testing a completely different thing. Could you add your new code as a new function, and leave the old FIXME alone until it's fixed?
Alternatively, if your patch actually fixes the FIXME, could you just replace the FIXME comment with a CHECK and leave the rest of this function alone?

254

Just for my own curiosity: this new test case is surely unaffected by this patch, right?

[tzik]

lib/Sema/SemaDecl.cpp

12757–12758

I think the previous implementation was incorrect.
Though computeNRVO clears ReturnStmt::NRVOCandidate when the corresponding variable is not NRVO variable, CGStmt checks both of ReturnStmt::NRVOCandidate and VarDecl::NRVOVariable anyway.
So, computeNRVO took no effect in the previous code, and the absence of computeNRVO here on function templates did not matter.
Note that there was no chance to call computeNRVO on function template elsewhere too.

OTOH in the new implementation, computeNRVO is necessary, and its absence on function templates matters.

We can remove computeNRVO here separately as a NFC patch and readd the new impl to the same place, but it's probably less readable, IMO.

test/CodeGenCXX/nrvo.cpp

139

My patch fixes the FIXME.
However, on the resulting code of NRVOing,

X y;
return y;

and

X x;
return x;

get to the same code and unified. And, the function is simplified as

X test3(bool B) {
  X x;
  return x;
}

without the if statement.
So, there will be nothing to CHECK left here if I leave the code as-is. I think that does not fit to the test scenario.

254

Hm, this is not so important anymore. This was to check if NRVO is working with function templates on the existing code, as computeNRVO was not called on them. And also this covered a regression in a draft patch, that didn't work on function templates.

[rsmith]

Thanks! This looks like exactly the right way to compute when to apply NRVO. It'd be good to track (in your commit message at least) that this addresses PR13067.

lib/Sema/SemaDecl.cpp

12757–12758

What happens if we can't tell whether we have an NRVO candidate or not until instantiation:

template<typename T> X f() {
  T v;
  return v; // is an NRVO candidate if T is X, otherwise is not
}

(I think this is not hard to handle: the dependent construct here can only affect whether you get NRVO at all, not which variable you perform NRVO on, but I want to check that it is handled properly.)

test/CodeGenCXX/nrvo.cpp

139

... and this is one reason why we don't generally like clang's IR generation tests to enable optimizations. Please consider starting a new test file to test the output from clang's frontend rather than the output from -O1.

[• Quuxplusone]

lib/Sema/SemaDecl.cpp

12757–12758

Ah, so if I understand correctly — which I probably don't —...
the parts of this diff related to isRecordType() make NRVO more reliable on template functions? Because the existing code has lots of checks for isRecordType() which means that they don't run for dependent (as-yet-unknown) types, even if those types are going to turn out to be record types occasionally?

I see that line 12838 runs computeNRVO unconditionally for *all* ObjCMethodDecls, even ones with scalar return types. So maybe running computeNRVO unconditionally right here would also be correct?
Datapoint: I made a patch to do nothing but replace this condition with if (Body) and to remove the similar condition guarding computeNRVO in Sema::ActOnBlockStmtExpr. It passed the Clang test suite with flying colors. I don't know if this means that the test suite is missing some tests, or if it means that the conditions are unnecessary.

IMHO if you're changing this anyway, it would be nice to put the remaining condition/optimization if (getReturnType().isScalarType()) return; inside computeNRVO itself, instead of repeating that condition at every call site.

[tzik]

Add AST test. computeNRVO unconditionally.

[tzik]

lib/Sema/SemaDecl.cpp

12757–12758

v here is marked as an NRVO variable in its function template AST, and that propagates to the instantiation if the return type is compatible.
https://github.com/llvm-mirror/clang/blob/9c82d4ff6015e4477e924c8a495a834c2fced29e/lib/Sema/SemaTemplateInstantiateDecl.cpp#L743

12757–12758

the parts of this diff related to isRecordType() [...]

Right, they look not RecordTypes in the template context.

Because the existing code has lots of checks for isRecordType() [...]

Hm, maybe. Some of isRecordType don't have dependent type handling around them...

running computeNRVO unconditionally right here would also be correct?

Updated. Agree, scalar types should work, IIUC.

test/CodeGenCXX/nrvo.cpp

139

OK, I added one.

[rsmith]

Some minor nits. I would also like to see a test for the unoptimized (-O0) IR generated by Clang for the case where there are two NRVO variables in the same function. Otherwise, this looks good to go!

lib/Sema/Scope.cpp

122–123

* on the right, please. Also auto -> Decl would be clearer and no longer. Is dyn_cast_or_null really necessary? (Can DeclsInScope contain nullptr?) I would expect that just dyn_cast would suffice.

lib/Sema/SemaDecl.cpp

12620–12624

* on the right, please.

[tzik]

style fix. -O0 IR test.

[tzik]

lib/Sema/Scope.cpp

122–123

Done! Right, contents in DeclsInScope should be non-null, and dyn_cast should work well.

lib/Sema/SemaDecl.cpp

12620–12624

Done

[xbolva00]

lib/Sema/Scope.cpp

128–129

auto * Parent = getParent();
if (Parent)

Parent>setNRVOCandidate(Candidate);

?

[tzik]

reuse getParent() result

[tzik]

lib/Sema/Scope.cpp

128–129

Updated!

[• Quuxplusone]

Just commenting to say that this LGTM and I have no further nitpicks. I have verified that I cannot detect any change in the behavior of -Wpessimizing-move or -Wreturn-std-move due to this change (and I can successfully detect the additional copy-elision due to this change, hooray).

[rsmith]

Thank you, do you need someone to commit this for you?

[tzik]

In D47067#1115566, @rsmith wrote:

Thank you, do you need someone to commit this for you?

No, I recently got the commit access to the repository.

[sammccall]

Unfortunately this seems to miscompile, the stage1 built clang is crashing on the multistage buildbots:

http://lab.llvm.org:8011/builders/clang-s390x-linux-multistage/builds/2926 shows this crash present already at 333500
I've locally verified the crash and that that reverting this patch fixes it.

I'm going to revert it.

[tzik]

In D47067#1116246, @sammccall wrote:

Unfortunately this seems to miscompile, the stage1 built clang is crashing on the multistage buildbots:

http://lab.llvm.org:8011/builders/clang-s390x-linux-multistage/builds/2926 shows this crash present already at 333500
I've locally verified the crash and that that reverting this patch fixes it.

I'm going to revert it.

Ah... Thanks for taking care.

[rsmith]

One of the sanitizer bots has more useful information on the problem: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/5743

[rsmith]

Based on the ASan output, it looks like the miscompile is probably happening in SparseSolver<LatticeKey, LatticeVal, KeyInfo>::getValueState(LatticeKey) at around include/llvm/Analysis/SparsePropagation.h:240:

template <class LatticeKey, class LatticeVal, class KeyInfo>
LatticeVal
SparseSolver<LatticeKey, LatticeVal, KeyInfo>::getValueState(LatticeKey Key) {
  auto I = ValueState.find(Key);
  if (I != ValueState.end())
    return I->second; // Common case, in the map

  if (LatticeFunc->IsUntrackedValue(Key))
    return LatticeFunc->getUntrackedVal();
  LatticeVal LV = LatticeFunc->ComputeLatticeVal(Key);

  // If this value is untracked, don't add it to the map.
  if (LV == LatticeFunc->getUntrackedVal())
    return LV;
  return ValueState[Key] = LV;
}

I'm guessing we somehow get confused about whether LV is an NRVO variable during template instantiation, and apply NRVO to it despite there being a return of something else in its scope.

[rsmith]

Slightly reduced testcase:

template<typename T> T f(T u, bool b) {
  T t;
  if (b) return t;
  return u;
}
struct X { X(); X(const X&); ~X(); void *data; };

template X f(X, bool);

... which compiles to:

X f(X u, bool b) {
  X::X(&return-slot);
  if (b) return;
  X::X(&return-slot, u);
  X::~X(&return-slot);
}

... due to t incorrectly being used as an NRVO variable.

[tzik]

In D47067#1116733, @rsmith wrote:

Slightly reduced testcase:

template<typename T> T f(T u, bool b) {
  T t;
  if (b) return t;
  return u;
}
struct X { X(); X(const X&); ~X(); void *data; };

template X f(X, bool);

... which compiles to:

X f(X u, bool b) {
  X::X(&return-slot);
  if (b) return;
  X::X(&return-slot, u);
  X::~X(&return-slot);
}

... due to t incorrectly being used as an NRVO variable.

Thanks for your investigation!
I thought that's covered by Scope::setNRVOCandidate with a null Candidate. As t is not marked as an NRVO variable before its instantiation. There's likely something I missed that turn the flag on while its instantiation.

include/clang/AST/Decl.h

protected:
 
   enum DefaultArgKind {
     DAK_None,
     DAK_Unparsed,
     DAK_Uninstantiated,
     DAK_Normal
   };
 
+  enum NRVOMode {
+    NRVO_Candidate,
+    NRVO_Disabled,
+    NRVO_Enabled,
+  };
+
   class ParmVarDeclBitfields {
     friend class ASTDeclReader;
     friend class ParmVarDecl;
 
     unsigned : NumVarDeclBits;
 
     /// Whether this parameter inherits a default argument from a
     /// prior declaration.
class NonParmVarDeclBitfields {
 
     /// Whether this variable is the exception variable in a C++ catch
     /// or an Objective-C @catch statement.
     unsigned ExceptionVar : 1;
 
     /// Whether this local variable could be allocated in the return
     /// slot of its function, enabling the named return value optimization
     /// (NRVO).
-    unsigned NRVOVariable : 1;
+    unsigned NRVOMode : 2;
 
     /// Whether this variable is the for-range-declaration in a C++0x
     /// for-range statement.
     unsigned CXXForRangeDecl : 1;
 
     /// Whether this variable is the for-in loop declaration in Objective-C.
     unsigned ObjCForDecl : 1;
 
public:
   ///
   /// The named return value optimization (NRVO) works by marking certain
   /// non-volatile local variables of class type as NRVO objects. These
   /// locals can be allocated within the return slot of their containing
   /// function, in which case there is no need to copy the object to the
   /// return slot when returning from the function. Within the function body,
   /// each return that returns the NRVO object will have this variable as its
   /// NRVO candidate.
+  NRVOMode getNRVOMode() const {
+    if (isa<ParmVarDecl>(this))
+      return NRVO_Disabled;
+    return static_cast<NRVOMode>(NonParmVarDeclBits.NRVOMode);
+  }
+  bool isNRVOCandidate() const {
+    return isa<ParmVarDecl>(this) ? false : NonParmVarDeclBits.NRVOMode == NRVO_Candidate;
+  }
   bool isNRVOVariable() const {
-    return isa<ParmVarDecl>(this) ? false : NonParmVarDeclBits.NRVOVariable;
+    return isa<ParmVarDecl>(this) ? false : NonParmVarDeclBits.NRVOMode == NRVO_Enabled;
   }
   void setNRVOVariable(bool NRVO) {
     assert(!isa<ParmVarDecl>(this));
-    NonParmVarDeclBits.NRVOVariable = NRVO;
+    NonParmVarDeclBits.NRVOMode = NRVO ? NRVO_Enabled : NRVO_Disabled;
   }
 
   /// Determine whether this variable is the for-range-declaration in
   /// a C++0x for-range statement.
   bool isCXXForRangeDecl() const {
     return isa<ParmVarDecl>(this) ? false : NonParmVarDeclBits.CXXForRangeDecl;
   }
   void setCXXForRangeDecl(bool FRD) {

include/clang/Sema/Scope.h

private:
   DeclContext *Entity;
 
   using UsingDirectivesTy = SmallVector<UsingDirectiveDecl *, 2>;
   UsingDirectivesTy UsingDirectives;
 
   /// Used to determine if errors occurred in this scope.
   DiagnosticErrorTrap ErrorTrap;
 
-  /// A lattice consisting of undefined, a single NRVO candidate variable in
-  /// this scope, or over-defined. The bit is true when over-defined.
-  llvm::PointerIntPair<VarDecl *, 1, bool> NRVO;
-
   void setFlags(Scope *Parent, unsigned F);
 
 public:
   Scope(Scope *Parent, unsigned ScopeFlags, DiagnosticsEngine &Diag)
       : ErrorTrap(Diag) {
     Init(Parent, ScopeFlags);
   }
 
public:
   using using_directives_range =
       llvm::iterator_range<UsingDirectivesTy::iterator>;
 
   using_directives_range using_directives() {
     return using_directives_range(UsingDirectives.begin(),
                                   UsingDirectives.end());
   }
 
-  void addNRVOCandidate(VarDecl *VD) {
-    if (NRVO.getInt())
-      return;
-    if (NRVO.getPointer() == nullptr) {
-      NRVO.setPointer(VD);
-      return;
-    }
-    if (NRVO.getPointer() != VD)
-      setNoNRVO();
-  }
-
-  void setNoNRVO() {
-    NRVO.setInt(true);
-    NRVO.setPointer(nullptr);
-  }
-
-  void mergeNRVOIntoParent();
+  void setNRVOCandidate(VarDecl *Candidate);
 
   /// Init - This is used by the parser to implement scope caching.
   void Init(Scope *parent, unsigned flags);
 
   /// Sets up the specified scope flags and adjusts the scope state
   /// variables accordingly.
   void AddFlags(unsigned Flags);
 
   void dumpImpl(raw_ostream &OS) const;
   void dump() const;
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_SEMA_SCOPE_H

lib/Sema/Scope.cpp

 
 void Scope::Init(Scope *parent, unsigned flags) {
   setFlags(parent, flags);
 
   DeclsInScope.clear();
   UsingDirectives.clear();
   Entity = nullptr;
   ErrorTrap.reset();
-  NRVO.setPointerAndInt(nullptr, 0);
 }
 
 bool Scope::containedInPrototypeScope() const {
   const Scope *S = this;
   while (S) {
     if (S->isFunctionPrototypeScope())
       return true;
     S = S->getParent();
void Scope::AddFlags(unsigned FlagsToSet) {
   }
   if (FlagsToSet & ContinueScope) {
     assert((Flags & ContinueScope) == 0 && "Already set");
     ContinueParent = this;
   }
   Flags |= FlagsToSet;
 }
 
-void Scope::mergeNRVOIntoParent() {
-  if (VarDecl *Candidate = NRVO.getPointer()) {
-    if (isDeclScope(Candidate))
-      Candidate->setNRVOVariable(true);
+void Scope::setNRVOCandidate(VarDecl *Candidate) {
+  for (Decl *D : DeclsInScope) {
+    VarDecl *VD = dyn_cast<VarDecl>(D);
+    if (VD && VD != Candidate && VD->isNRVOCandidate())
+      VD->setNRVOVariable(false);
   }
 
-  if (getEntity())
-    return;
-
-  if (NRVO.getInt())
-    getParent()->setNoNRVO();
-  else if (NRVO.getPointer())
-    getParent()->addNRVOCandidate(NRVO.getPointer());
+  if (Scope *parent = getParent())
+    parent->setNRVOCandidate(Candidate);
 }
 
 LLVM_DUMP_METHOD void Scope::dump() const { dumpImpl(llvm::errs()); }
 
 void Scope::dumpImpl(raw_ostream &OS) const {
   unsigned Flags = getFlags();
   bool HasFlags = Flags != 0;
 
void Scope::dumpImpl(raw_ostream &OS) const {
   if (const Scope *Parent = getParent())
     OS << "Parent: (clang::Scope*)" << Parent << '\n';
 
   OS << "Depth: " << Depth << '\n';
   OS << "MSLastManglingNumber: " << getMSLastManglingNumber() << '\n';
   OS << "MSCurManglingNumber: " << getMSCurManglingNumber() << '\n';
   if (const DeclContext *DC = getEntity())
     OS << "Entity : (clang::DeclContext*)" << DC << '\n';
-
-  if (NRVO.getInt())
-    OS << "NRVO not allowed\n";
-  else if (NRVO.getPointer())
-    OS << "NRVO candidate : (clang::VarDecl*)" << NRVO.getPointer() << '\n';
 }

lib/Sema/SemaDecl.cpp

if (L->isMSAsmLabel())
     Diagnose = !L->isResolvedMSAsmLabel();
   else
     Diagnose = L->getStmt() == nullptr;
   if (Diagnose)
     S.Diag(L->getLocation(), diag::err_undeclared_label_use) <<L->getDeclName();
 }
 
 void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
-  S->mergeNRVOIntoParent();
-
   if (S->decl_empty()) return;
   assert((S->getFlags() & (Scope::DeclScope | Scope::TemplateParamScope)) &&
          "Scope shouldn't contain decls!");
 
   for (auto *TmpD : S->decls()) {
     assert(TmpD && "This decl didn't get pushed??");
 
     assert(isa<NamedDecl>(TmpD) && "Decl isn't NamedDecl?");
Decl *Sema::ActOnStartOfFunctionDef(Scope *FnBodyScope, Decl *D,
   return D;
 }
 
 /// Given the set of return statements within a function body,
 /// compute the variables that are subject to the named return value
 /// optimization.
 ///
 /// Each of the variables that is subject to the named return value
-/// optimization will be marked as NRVO variables in the AST, and any
+/// optimization will be marked as NRVO variable candidates in the AST, and any
 /// return statement that has a marked NRVO variable as its NRVO candidate can
 /// use the named return value optimization.
 ///
-/// This function applies a very simplistic algorithm for NRVO: if every return
-/// statement in the scope of a variable has the same NRVO candidate, that
-/// candidate is an NRVO variable.
+/// This function applies a very simplistic algorithm for NRVO: if every
+/// reachable return statement in the scope of a variable has the same NRVO
+/// candidate, that candidate is an NRVO variable.
 void Sema::computeNRVO(Stmt *Body, FunctionScopeInfo *Scope) {
-  ReturnStmt **Returns = Scope->Returns.data();
+  for (ReturnStmt *Return : Scope->Returns) {
+    const VarDecl *Candidate = Return->getNRVOCandidate();
+    if (!Candidate)
+      continue;
 
-  for (unsigned I = 0, E = Scope->Returns.size(); I != E; ++I) {
-    if (const VarDecl *NRVOCandidate = Returns[I]->getNRVOCandidate()) {
-      if (!NRVOCandidate->isNRVOVariable())
-        Returns[I]->setNRVOCandidate(nullptr);
-    }
+    if (Candidate->isNRVOCandidate())
+      const_cast<VarDecl*>(Candidate)->setNRVOVariable(true);
+
+    if (!Candidate->isNRVOVariable())
+      Return->setNRVOCandidate(nullptr);
   }
 }
 
 bool Sema::canDelayFunctionBody(const Declarator &D) {
   // We can't delay parsing the body of a constexpr function template (yet).
   if (D.getDeclSpec().isConstexprSpecified())
     return false;
 
if (!FD->isInvalidDecl()) {
       DiagnoseSizeOfParametersAndReturnValue(FD->parameters(),
                                              FD->getReturnType(), FD);
 
       // If this is a structor, we need a vtable.
       if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(FD))
         MarkVTableUsed(FD->getLocation(), Constructor->getParent());
       else if (CXXDestructorDecl *Destructor = dyn_cast<CXXDestructorDecl>(FD))
         MarkVTableUsed(FD->getLocation(), Destructor->getParent());
 
-      // Try to apply the named return value optimization. We have to check
-      // if we can do this here because lambdas keep return statements around
-      // to deduce an implicit return type.
-      if (FD->getReturnType()->isRecordType() &&
-          (!getLangOpts().CPlusPlus || !FD->isDependentContext()))
-        computeNRVO(Body, getCurFunction());
+      // Try to apply the named return value optimization.
     }
 
     // GNU warning -Wmissing-prototypes:
     //   Warn if a global function is defined without a previous
     //   prototype declaration. This warning is issued even if the
     //   definition itself provides a prototype. The aim is to detect
     //   global functions that fail to be declared in header files.
     const FunctionDecl *PossibleZeroParamPrototype = nullptr;

lib/Sema/SemaExpr.cpp

if (getCurFunction()->NeedsScopeChecking() &&
       !PP.isCodeCompletionEnabled())
     DiagnoseInvalidJumps(cast<CompoundStmt>(Body));
 
   BSI->TheDecl->setBody(cast<CompoundStmt>(Body));
 
   if (Body && getCurFunction()->HasPotentialAvailabilityViolations)
     DiagnoseUnguardedAvailabilityViolations(BSI->TheDecl);
 
-  // Try to apply the named return value optimization. We have to check again
-  // if we can do this, though, because blocks keep return statements around
-  // to deduce an implicit return type.
-  if (getLangOpts().CPlusPlus && RetTy->isRecordType() &&
-      !BSI->TheDecl->isDependentContext())
-    computeNRVO(Body, BSI);
-  
+  // Try to apply the named return value optimization.
   BlockExpr *Result = new (Context) BlockExpr(BSI->TheDecl, BlockTy);
   AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy();
   PopFunctionScopeInfo(&WP, Result->getBlockDecl(), Result);
 
   // If the block isn't obviously global, i.e. it captures anything at
   // all, then we need to do a few things in the surrounding context:
   if (Result->getBlockDecl()->hasCaptures()) {
     // First, this expression has a new cleanup object.

lib/Sema/SemaStmt.cpp

 StmtResult
 Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
                       Scope *CurScope) {
   StmtResult R = BuildReturnStmt(ReturnLoc, RetValExp);
   if (R.isInvalid() || ExprEvalContexts.back().Context ==
                            ExpressionEvaluationContext::DiscardedStatement)
     return R;
 
-  if (VarDecl *VD =
-      const_cast<VarDecl*>(cast<ReturnStmt>(R.get())->getNRVOCandidate())) {
-    CurScope->addNRVOCandidate(VD);
-  } else {
-    CurScope->setNoNRVO();
-  }
+  VarDecl *VD =
+      const_cast<VarDecl*>(cast<ReturnStmt>(R.get())->getNRVOCandidate());
+  CurScope->setNRVOCandidate(VD);
 
   CheckJumpOutOfSEHFinally(*this, ReturnLoc, *CurScope->getFnParent());
 
   return R;
 }
 
 StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
   // Check for unexpanded parameter packs.

lib/Serialization/ASTReaderDecl.cpp

ASTDeclReader::RedeclarableResult ASTDeclReader::VisitVarDeclImpl(VarDecl *VD) {
 
   VD->VarDeclBits.SClass = (StorageClass)Record.readInt();
   VD->VarDeclBits.TSCSpec = Record.readInt();
   VD->VarDeclBits.InitStyle = Record.readInt();
   if (!isa<ParmVarDecl>(VD)) {
     VD->NonParmVarDeclBits.IsThisDeclarationADemotedDefinition =
         Record.readInt();
     VD->NonParmVarDeclBits.ExceptionVar = Record.readInt();
-    VD->NonParmVarDeclBits.NRVOVariable = Record.readInt();
+    VD->NonParmVarDeclBits.NRVOMode = Record.readInt();
     VD->NonParmVarDeclBits.CXXForRangeDecl = Record.readInt();
     VD->NonParmVarDeclBits.ObjCForDecl = Record.readInt();
     VD->NonParmVarDeclBits.ARCPseudoStrong = Record.readInt();
     VD->NonParmVarDeclBits.IsInline = Record.readInt();
     VD->NonParmVarDeclBits.IsInlineSpecified = Record.readInt();
     VD->NonParmVarDeclBits.IsConstexpr = Record.readInt();
     VD->NonParmVarDeclBits.IsInitCapture = Record.readInt();
     VD->NonParmVarDeclBits.PreviousDeclInSameBlockScope = Record.readInt();

lib/Serialization/ASTWriterDecl.cpp

void ASTDeclWriter::VisitVarDecl(VarDecl *D) {
   VisitRedeclarable(D);
   VisitDeclaratorDecl(D);
   Record.push_back(D->getStorageClass());
   Record.push_back(D->getTSCSpec());
   Record.push_back(D->getInitStyle());
   if (!isa<ParmVarDecl>(D)) {
     Record.push_back(D->isThisDeclarationADemotedDefinition());
     Record.push_back(D->isExceptionVariable());
-    Record.push_back(D->isNRVOVariable());
+    Record.push_back(D->getNRVOMode());
     Record.push_back(D->isCXXForRangeDecl());
     Record.push_back(D->isObjCForDecl());
     Record.push_back(D->isARCPseudoStrong());
     Record.push_back(D->isInline());
     Record.push_back(D->isInlineSpecified());
     Record.push_back(D->isConstexpr());
     Record.push_back(D->isInitCapture());
     Record.push_back(D->isPreviousDeclInSameBlockScope());
void ASTWriter::WriteDeclAbbrevs() {
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // InnerStartLoc
   Abv->Add(BitCodeAbbrevOp(0));                       // hasExtInfo
   // VarDecl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // SClass
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // TSCSpec
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // InitStyle
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsThisDeclarationADemotedDefinition
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isExceptionVariable
-  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isNRVOVariable
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // NRVOMode
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isCXXForRangeDecl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isObjCForDecl
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong
   Abv->Add(BitCodeAbbrevOp(0));                         // isInline
   Abv->Add(BitCodeAbbrevOp(0));                         // isInlineSpecified
   Abv->Add(BitCodeAbbrevOp(0));                         // isConstexpr
   Abv->Add(BitCodeAbbrevOp(0));                         // isInitCapture
   Abv->Add(BitCodeAbbrevOp(0));                         // isPrevDeclInSameScope

test/CodeGenCXX/nrvo-noopt.cpp

+// RUN: %clang_cc1 -emit-llvm -O0 -o - %s | FileCheck %s
+
+struct X {
+  X();
+  X(X&&);
+};
+
+// CHECK-LABEL: define void @_Z7test_00b
+X test_00(bool b) {
+  if (b) {
+    // CHECK-NOT: call void @_ZN1XC1EOS_
+    // CHECK: call void @_ZN1XC1Ev
+    // CHECK-NEXT: br label %return
+    X x;
+    return x;
+  } else {
+    // CHECK-NOT: call void @_ZN1XC1EOS_
+    // CHECK: call void @_ZN1XC1Ev
+    // CHECK-NEXT: br label %return
+    X x;
+    return x;
+  }
+}
+
+// CHECK-LABEL: define void @_Z7test_01b
+X test_01(bool b) {
+  if (b) {
+    // CHECK-NOT: call void @_ZN1XC1EOS_
+    // CHECK: call void @_ZN1XC1Ev
+    // CHECK-NEXT: br label %return
+    X x;
+    return x;
+  }
+  // CHECK-NOT: call void @_ZN1XC1EOS_
+  // CHECK: call void @_ZN1XC1Ev
+  // CHECK-NEXT: br label %return
+  X x;
+  return x;
+}
+
+// CHECK-LABEL: define void @_Z7test_02b
+X test_02(bool b) {
+  // CHECK: call void @_ZN1XC1Ev
+  X x;
+
+  if (b) {
+    // CHECK-NOT: call void @_ZN1XC1EOS_
+    // CHECK: call void @_ZN1XC1Ev
+    // CHECK-NEXT: br label %return
+    X y;
+    return y;
+  }
+
+  // CHECK-NOT: call void @_ZN1XC1Ev
+  // CHECK: call void @_ZN1XC1EOS_
+  // CHECK-NEXT: br label %return
+  return x;
+}

test/CodeGenCXX/nrvo.cpp

X test2(bool B) {
   // CHECK-EH-03-NEXT:   catch i8* null
   // CHECK-EH-03-NEXT: [[T1:%.*]] = extractvalue { i8*, i32 } [[T0]], 0
   // CHECK-EH-03-NEXT: call void @__clang_call_terminate(i8* [[T1]]) [[NR_NUW:#[0-9]+]]
   // CHECK-EH-03-NEXT: unreachable
 
 }
 
 // CHECK-LABEL: define void @_Z5test3b
-X test3(bool B) {
+X test3(bool B, X x) {
   // CHECK: tail call {{.*}} @_ZN1XC1Ev
-  // CHECK-NOT: call {{.*}} @_ZN1XC1ERKS_
-  // CHECK: call {{.*}} @_ZN1XC1Ev
-  // CHECK: call {{.*}} @_ZN1XC1ERKS_
   if (B) {
     X y;
     return y;
   }
-  // FIXME: we should NRVO this variable too.
-  X x;
+  // CHECK: tail call {{.*}} @_ZN1XC1ERKS_
   return x;
 }
 
 extern "C" void exit(int) throw();
 
 // CHECK-LABEL: define void @_Z5test4b
 X test4(bool B) {
   {
X test6() {
   // CHECK-NEXT: call {{.*}} @_ZN1XC1Ev([[X]]* nonnull [[A]])
   // CHECK-NEXT: call {{.*}} @_ZN1XC1ERKS_([[X]]* {{%.*}}, [[X]]* nonnull dereferenceable({{[0-9]+}}) [[A]])
   // CHECK-NEXT: call {{.*}} @_ZN1XD1Ev([[X]]* nonnull [[A]])
   // CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull [[PTR]])
   // CHECK-NEXT: ret void
 }
 
 // CHECK-LABEL: define void @_Z5test7b
+// CHECK-EH-LABEL: define void @_Z5test7b
 X test7(bool b) {
   // CHECK: tail call {{.*}} @_ZN1XC1Ev
   // CHECK-NEXT: ret
+
+  // CHECK-EH: tail call {{.*}} @_ZN1XC1Ev
+  // CHECK-EH-NEXT: ret
   if (b) {
     X x;
     return x;
   }
   return X();
 }
 
 // CHECK-LABEL: define void @_Z5test8b
+// CHECK-EH-LABEL: define void @_Z5test8b
 X test8(bool b) {
   // CHECK: tail call {{.*}} @_ZN1XC1Ev
   // CHECK-NEXT: ret
-  if (b) {
+
+  // CHECK-EH: tail call {{.*}} @_ZN1XC1Ev
+  // CHECK-EH-NEXT: ret
     X x;
     return x;
   } else {
     X y;
     return y;
   }
 }
 
 Y<int> test9() {
   Y<int>::f();
 }
 
 // CHECK-LABEL: define linkonce_odr void @_ZN1YIiE1fEv
 // CHECK: tail call {{.*}} @_ZN1YIiEC1Ev
 
+// CHECK-LABEL: define void @_Z6test10b
+X test10(bool B, X x) {
+  if (B) {
+    // CHECK: tail call {{.*}} @_ZN1XC1ERKS_
+    // CHECK-EH: tail call {{.*}} @_ZN1XC1ERKS_
+    return x;
+  }
+  // CHECK: tail call {{.*}} @_ZN1XC1Ev
+  // CHECK-NOT: call {{.*}} @_ZN1XC1ERKS_
+
+  // CHECK-EH: tail call {{.*}} @_ZN1XC1Ev
+  // CHECK-EH-NOT: call {{.*}} @_ZN1XC1ERKS_
+  X y;
+  return y;
+}
+
+// CHECK-LABEL: define {{.*}} void @_Z6test11I1XET_v
+// CHECK-EH-LABEL: define {{.*}} void @_Z6test11I1XET_v
+template <typename T>
+T test11() {
+  // CHECK:      tail call {{.*}} @_ZN1XC1Ev
+  // CHECK-NEXT: ret void
+
+  // CHECK-EH:      tail call {{.*}} @_ZN1XC1Ev
+  // CHECK-EH-NEXT: ret void
+  T t;
+  return t;
+}
+
+void test12() {
+  test11<X>();
+}
+
 // CHECK-EH-03: attributes [[NR_NUW]] = { noreturn nounwind }

test/SemaCXX/nrvo-ast.cpp

+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -ast-dump -o - %s | FileCheck %s
+
+struct X {
+  X();
+  X(const X&);
+  X(X&&);
+};
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_00
+X test_00() {
+  // CHECK: VarDecl {{.*}} x {{.*}} nrvo
+  X x;
+  return x;
+}
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_01
+X test_01(bool b) {
+  // CHECK: VarDecl {{.*}} x {{.*}} nrvo
+  X x;
+  if (b)
+    return x;
+  return x;
+}
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_02
+X test_02(bool b) {
+  // CHECK-NOT: VarDecl {{.*}} x {{.*}} nrvo
+  X x;
+  // CHECK-NOT: VarDecl {{.*}} y {{.*}} nrvo
+  X y;
+  if (b)
+    return y;
+  return x;
+}
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_03
+X test_03(bool b) {
+  if (b) {
+    // CHECK: VarDecl {{.*}} y {{.*}} nrvo
+    X y;
+    return y;
+  }
+  // CHECK: VarDecl {{.*}} x {{.*}} nrvo
+  X x;
+  return x;
+}
+
+extern "C" _Noreturn void exit(int) throw();
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_04
+X test_04(bool b) {
+  {
+    // CHECK: VarDecl {{.*}} x {{.*}} nrvo
+    X x;
+    if (b)
+      return x;
+  }
+  exit(1);
+}
+
+void may_throw();
+// CHECK-LABEL: FunctionDecl {{.*}} test_05
+X test_05() {
+  try {
+    may_throw();
+    return X();
+  } catch (X x) {
+    // CHECK-NOT: VarDecl {{.*}} x {{.*}} nrvo
+    return x;
+  }
+}
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_06
+X test_06() {
+  // CHECK-NOT: VarDecl {{.*}} x {{.*}} nrvo
+  X x __attribute__((aligned(8)));
+  return x;
+}
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_07
+X test_07(bool b) {
+  if (b) {
+    // CHECK: VarDecl {{.*}} x {{.*}} nrvo
+    X x;
+    return x;
+  }
+  return X();
+}
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_08
+X test_08(bool b) {
+  if (b) {
+    // CHECK: VarDecl {{.*}} x {{.*}} nrvo
+    X x;
+    return x;
+  } else {
+    // CHECK: VarDecl {{.*}} y {{.*}} nrvo
+    X y;
+    return y;
+  }
+}
+
+template <typename T>
+struct Y {
+  Y();
+  // CHECK-LABEL: CXXMethodDecl {{.*}} test_09 'Y<T> ()'
+  // CHECK: VarDecl {{.*}} y 'Y<T>' nrvo
+
+  // CHECK-LABEL: CXXMethodDecl {{.*}} test_09 'Y<int> ()'
+  // CHECK: VarDecl {{.*}} y 'Y<int>' nrvo
+  static Y test_09() {
+    Y y;
+    return y;
+  }
+};
+
+struct Z {
+  Z(const X&);
+};
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_10 'A ()'
+// CHECK: VarDecl {{.*}} b 'B' nrvo
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_10 'X ()'
+// CHECK: VarDecl {{.*}} b {{.*}} nrvo
+
+// CHECK-LABEL: FunctionDecl {{.*}} test_10 'Z ()'
+// CHECK-NOT: VarDecl {{.*}} b {{.*}} nrvo
+template <typename A, typename B>
+A test_10() {
+  B b;
+  return b;
+}
+
+void instantiate() {
+  Y<int>::test_09();
+  test_10<X, X>();
+  test_10<Z, X>();
+}