[PATCH] D47067: Update NRVO logic to support early return

tzik added a comment. In https://reviews.llvm.org/D47067#1116733, @rsmith wrote: > Slightly reduced testcase: > > template 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); > > > ...

[PATCH] D47067: Update NRVO logic to support early return

rsmith added a comment. Slightly reduced testcase: template 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(); if (b) return; X::X(,

[PATCH] D47067: Update NRVO logic to support early return

rsmith added a comment. Based on the ASan output, it looks like the miscompile is probably happening in `SparseSolver::getValueState(LatticeKey)` at around include/llvm/Analysis/SparsePropagation.h:240: template LatticeVal SparseSolver::getValueState(LatticeKey Key) { auto I =

[PATCH] D47067: Update NRVO logic to support early return

rsmith added a comment. 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 Repository: rC Clang https://reviews.llvm.org/D47067 ___ cfe-commits

[PATCH] D47067: Update NRVO logic to support early return

tzik added a comment. In https://reviews.llvm.org/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

[PATCH] D47067: Update NRVO logic to support early return

sammccall added a comment. 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

[PATCH] D47067: Update NRVO logic to support early return

tzik added a comment. In https://reviews.llvm.org/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. Repository: rC Clang https://reviews.llvm.org/D47067

[PATCH] D47067: Update NRVO logic to support early return

This revision was automatically updated to reflect the committed changes. Closed by commit rC333500: Update NRVO logic to support early return (authored by tzik, committed by ). Changed prior to commit: https://reviews.llvm.org/D47067?vs=148603=149035#toc Repository: rC Clang

[PATCH] D47067: Update NRVO logic to support early return

rsmith added a comment. Thank you, do you need someone to commit this for you? Repository: rC Clang https://reviews.llvm.org/D47067 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47067: Update NRVO logic to support early return

Quuxplusone added a comment. 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

[PATCH] D47067: Update NRVO logic to support early return

tzik added inline comments. Comment at: lib/Sema/Scope.cpp:128 - if (getEntity()) -return; - - if (NRVO.getInt()) -getParent()->setNoNRVO(); - else if (NRVO.getPointer()) -getParent()->addNRVOCandidate(NRVO.getPointer()); + if (getParent()) +

[PATCH] D47067: Update NRVO logic to support early return

tzik updated this revision to Diff 148603. tzik added a comment. reuse getParent() result Repository: rC Clang https://reviews.llvm.org/D47067 Files: include/clang/AST/Decl.h include/clang/Sema/Scope.h lib/Sema/Scope.cpp lib/Sema/SemaDecl.cpp lib/Sema/SemaExpr.cpp

[PATCH] D47067: Update NRVO logic to support early return

xbolva00 added inline comments. Comment at: lib/Sema/Scope.cpp:128 - if (getEntity()) -return; - - if (NRVO.getInt()) -getParent()->setNoNRVO(); - else if (NRVO.getPointer()) -getParent()->addNRVOCandidate(NRVO.getPointer()); + if (getParent()) +

[PATCH] D47067: Update NRVO logic to support early return

tzik added inline comments. Comment at: lib/Sema/Scope.cpp:122-123 +void Scope::setNRVOCandidate(VarDecl *Candidate) { + for (auto* D : DeclsInScope) { +VarDecl* VD = dyn_cast_or_null(D); +if (VD && VD != Candidate && VD->isNRVOCandidate()) rsmith

[PATCH] D47067: Update NRVO logic to support early return

tzik updated this revision to Diff 148570. tzik marked 2 inline comments as done. tzik added a comment. style fix. -O0 IR test. Repository: rC Clang https://reviews.llvm.org/D47067 Files: include/clang/AST/Decl.h include/clang/Sema/Scope.h lib/Sema/Scope.cpp lib/Sema/SemaDecl.cpp

[PATCH] D47067: Update NRVO logic to support early return

rsmith added a comment. 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! Comment at: lib/Sema/Scope.cpp:122-123 +void

[PATCH] D47067: Update NRVO logic to support early return

tzik added inline comments. Comment at: lib/Sema/SemaDecl.cpp:12760 // to deduce an implicit return type. - if (FD->getReturnType()->isRecordType() && - (!getLangOpts().CPlusPlus || !FD->isDependentContext())) + if (!FD->getReturnType()->isScalarType())

[PATCH] D47067: Update NRVO logic to support early return

tzik updated this revision to Diff 148417. tzik added a comment. Add AST test. computeNRVO unconditionally. Repository: rC Clang https://reviews.llvm.org/D47067 Files: include/clang/AST/Decl.h include/clang/Sema/Scope.h lib/Sema/Scope.cpp lib/Sema/SemaDecl.cpp

[PATCH] D47067: Update NRVO logic to support early return

Quuxplusone added inline comments. Comment at: lib/Sema/SemaDecl.cpp:12760 // to deduce an implicit return type. - if (FD->getReturnType()->isRecordType() && - (!getLangOpts().CPlusPlus || !FD->isDependentContext())) + if

[PATCH] D47067: Update NRVO logic to support early return

rsmith added a comment. 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. Comment at: lib/Sema/SemaDecl.cpp:12760 // to deduce an implicit return type. -

[PATCH] D47067: Update NRVO logic to support early return

tzik added inline comments. Comment at: lib/Sema/SemaDecl.cpp:12760 // to deduce an implicit return type. - if (FD->getReturnType()->isRecordType() && - (!getLangOpts().CPlusPlus || !FD->isDependentContext())) + if (!FD->getReturnType()->isScalarType())

[PATCH] D47067: Update NRVO logic to support early return

Quuxplusone added inline comments. Comment at: lib/Sema/SemaDecl.cpp:12760 // to deduce an implicit return type. - if (FD->getReturnType()->isRecordType() && - (!getLangOpts().CPlusPlus || !FD->isDependentContext())) + if

Re: [PATCH] D47067: Update NRVO logic to support early return

On Fri, May 18, 2018 at 8:47 PM, tzik via cfe-commits wrote: > Ah, sorry. It's still work in progress for now. > I'll resend this soon after adding tests and updating comments there. See Herald rules for exact syntax, but i think adding "[private]" into the subject

Re: [PATCH] D47067: Update NRVO logic to support early return

Ah, sorry. It's still work in progress for now. I'll resend this soon after adding tests and updating comments there. 2018年5月19日(土) 2:41 Arthur O'Dwyer : > https://reviews.llvm.org/D47067 is a "Restricted Differential Revision", > which I've never seen before! > Peanut

Re: [PATCH] D47067: Update NRVO logic to support early return

https://reviews.llvm.org/D47067 is a "Restricted Differential Revision", which I've never seen before! Peanut gallery says: This sounds awesome. How does this change interact with the diagnostics issued by -Wpessimizing-move and -Wreturn-std-move? Should any new test cases be added for those

[PATCH] D47067: Update NRVO logic to support early return

tzik created this revision. Herald added a subscriber: cfe-commits. The previous implementation misses an opportunity to apply NRVO (Named Return Value Optimization) below. That discourages user to write early return code. --