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

anhtuyen added a comment. In D99696#2856370 , @mizvekov wrote: > @anhtuyen > > I pushed a DR with a preliminary fix for it: https://reviews.llvm.org/D105380 > > This is not ready to merge, still have to add test cases and also decide > between a pointed f

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

mizvekov added a comment. @anhtuyen I pushed a DR with a preliminary fix for it: https://reviews.llvm.org/D105380 This is not ready to merge, still have to add test cases and also decide between a pointed fix like this, or improving the ergonomics of `getDeclAlign` by returning possible failur

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

mizvekov added a comment. In D99696#2856285 , @anhtuyen wrote: > Hi Matheus @mizvekov, > The commit 12c90e2e25dfd1d38250055efc21acb42d158912 > from > this patch seems to cause a regres

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

anhtuyen added a comment. Hi Mathew @mizvekov, The commit 12c90e2e25dfd1d38250055efc21acb42d158912 from this patch seems to cause a regression, where an assertion failure starts to occur with testcases such as template in

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG12c90e2e25df: [clang] NRVO: Improvements and handling of more cases. (authored by mizvekov). Changed prior to commit: https://reviews.llvm.org/D99

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

mizvekov updated this revision to Diff 352566. mizvekov added a comment. Fix Arthur's nits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ https://reviews.llvm.org/D99696 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/Sema.cpp

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

Quuxplusone added a comment. Some tiny nits. I can't think of any interesting block-related cases (but my knowledge of blocks is apparently very rusty at this point). Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1100 +// This is the last chance we have of che

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

mizvekov added a comment. @rjmccall Added some tests covering these semantics: clang/test/SemaObjCXX/block-capture.mm and also some extra documentation to checkEscapingByref. Not sure if I got the terminology right here.. @Quuxplusone any other interesting corner cases for these new tests? Rep

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

mizvekov updated this revision to Diff 352271. mizvekov added a comment. -bugs +tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ https://reviews.llvm.org/D99696 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/Sema.cpp cla

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

mizvekov added a comment. In D99696#2817126 , @rjmccall wrote: > When `__block` variables get moved to the heap, they're supposed to be moved > if possible, not copied. It looks like PerformMoveOrCopyInitialization > requires NRVO info to be passed in t

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

rjmccall added a comment. When `__block` variables get moved to the heap, they're supposed to be moved if possible, not copied. It looks like PerformMoveOrCopyInitialization requires NRVO info to be passed in to ever do a move. Maybe it's being passed in wrong when building `__block` copy exp

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

hans added a comment. Since it seems more discussion is needed here, I've reverted in c60dd3b2626a4d9eefd9f82f9a406b0d28d3fd72 . Since they were hard to tease apart, the revert is for both D99696

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

hans added a comment. > @hans: FYI, that looks related to the immediately following D99005 > , not D99696 > specifically. No, my test case passes at 0d9e8f5f4b68252c6caa1ef81a30777b2f5d7242

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

Quuxplusone added a comment. @hans: FYI, that looks related to the immediately following D99005 , not D99696 specifically. I suspect that reverting D99005 (but leaving D99696

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

hans added subscribers: rjmccall, hans. hans added a comment. We're seeing new build errors in Chromium after this (http://crbug.com/1219457). Here's a reduced example: $ cat /tmp/a.mm #include struct Callback { // Move-only! Callback(const Callback&) = delete; Callback& op

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG1e50c3d785f4: [clang] NRVO: Improvements and handling of more cases. (authored by mizvekov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ https:

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

mizvekov updated this revision to Diff 351462. mizvekov added a comment. Look through AttributedType when obtaining FunctionDecl return type. Adds a couple more test cases to cover this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ htt

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

mizvekov reopened this revision. mizvekov added a comment. This revision is now accepted and ready to land. Thank you @stella.stamenova and @aeubanks for reporting and reverting this. Turns out was a silly mistake where we were not digging down through AttributedType nodes in order to get the fu

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

aeubanks added a comment. Crashes on a stage 2 build on Windows: ../rel/bin/clang-cl /nologo /showIncludes /Foobj/llvm/lib/MC/MCParser/MCParser.MasmParser.obj /c ../../llvm/lib/MC/MCParser/MasmParser.cpp -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT

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

stella.stamenova added a comment. It looks like this caused a couple of failures on the Windows LLDB bot due to crashes in clang: https://lab.llvm.org/buildbot/#/builders/83/builds/7142 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ htt

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG667fbcdd0b2e: [clang] NRVO: Improvements and handling of more cases. (authored by mizvekov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ https:

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

aaronpuchert added a comment. In D99696#2808592 , @Quuxplusone wrote: > @aaronpuchert what's your take on D99696 at > this point; is it good to go or still unresolved issues? Overall this looks sensible to me, and I don'

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

mizvekov added a comment. In D99696#2808592 , @Quuxplusone wrote: > @mizvekov, my understanding is: > > - D99696 was temporarily blocked on D103720 > , but now D103720 >

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

Quuxplusone accepted this revision. Quuxplusone added a comment. This revision is now accepted and ready to land. @mizvekov, my understanding is: - D99696 was temporarily blocked on D103720 , but now D103720

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

mizvekov added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ https://reviews.llvm.org/D99696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

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

mizvekov updated this revision to Diff 337903. mizvekov added a comment. - Added doc to disallowNRVO - Also detect implicit return type for blocks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ https://reviews.llvm.org/D99696 Files: c

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

aaronpuchert added inline comments. 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 ?

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

Quuxplusone added inline comments. 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 ? S

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

mizvekov added inline comments. 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

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

aaronpuchert added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3144-3145 +/// \param ReturnType This is the return type of the function. +/// \returns The copy elision candidate, in case the initial return expression +/// was copy elidable, or nullptr otherwise. +con

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

mizvekov updated this revision to Diff 337266. mizvekov added a comment. - Changed the downgrade* function name to disallowNRVO, given second thought, I think it's appropriate name. - Change to one variable declaration per statement as per Arthur's review. Repository: rG LLVM Github Monorepo

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

mizvekov added a comment. @rsmith: Actually, there is still something not quite right with this patch that I have to finish investigating. The B5 test should not have gotten copy elision. I thought at first that we had gotten the function return type deduced durin

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

mizvekov added inline comments. 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

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

mizvekov added inline comments. 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

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

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3084-3085 + bool ForceCXX20) { + bool hasCXX11 = getLangOpts().CPlusPlus11 || ForceCXX20, + hasCXX20 = getLangOpts().CPlusPlus20 || ForceCXX20; + Named

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

mizvekov added inline comments. 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

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

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,

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

mizvekov updated this revision to Diff 337248. mizvekov marked 7 inline comments as done. mizvekov added a comment. - Address rsmith review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ https://reviews.llvm.org/D99696 Files:

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

rsmith added inline comments. 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 inst

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

mizvekov added inline comments. 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 in

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

mizvekov added inline comments. 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 in

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

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, -

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

mizvekov marked an inline comment as done. mizvekov added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3140 +/// \param ReturnType This is the return type of the function. +void Sema::updNRVOResultWithRetType(NRVOResult &Res, QualType ReturnType) { + if (!Res.Candida

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

mizvekov updated this revision to Diff 336736. mizvekov marked an inline comment as done. mizvekov added a comment. - Removes code that tried to allow copy elision for functions with dependent 'auto' return types. The reason is explained in new comments. Will try to address these in future work.

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

aaronpuchert added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586 InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl); - ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType, -

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

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3140 +/// \param ReturnType This is the return type of the function. +void Sema::updNRVOResultWithRetType(NRVOResult &Res, QualType ReturnType) { + if (!Res.Candidate) mizvekov wrote: > a

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

mizvekov added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3140 +/// \param ReturnType This is the return type of the function. +void Sema::updNRVOResultWithRetType(NRVOResult &Res, QualType ReturnType) { + if (!Res.Candidate) aaronpuchert wrote: >

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

aaronpuchert added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3030-3032 + Res.S = Res.S != Sema::NRVOResult::None && CanMove + ? Sema::NRVOResult::MoveEligible + : Sema::NRVOResult::None; mizvekov wrote: > aaronpuchert wro

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

mizvekov marked 8 inline comments as done. mizvekov added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3058 -bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD, - CopyElisionSemanticsKind CESK) { - QualType VDT

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

mizvekov updated this revision to Diff 336626. mizvekov added a comment. - Addresses aaronpuchert's review points. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ https://reviews.llvm.org/D99696 Files: clang/include/clang/Sema/Sema.h

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

mizvekov added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586 InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl); - ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType, -

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

mizvekov added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:854 if (Ex && !Ex->isTypeDependent()) { +NRVOResult NRVORes = IsThrownVarInScope ? getNRVOResult(Ex) : NRVOResult(); + mizvekov wrote: > aaronpuchert wrote: > > Any reason why you'v

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

mizvekov added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586 InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl); - ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType, -

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

mizvekov added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586 InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl); - ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType, -

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

aaronpuchert added a comment. Didn't really check for correctness yet, just a superficial review. I like the idea, splitting the functionality up definitely helps understanding this. Comment at: clang/include/clang/Sema/Sema.h:4733 + +bool isMoveEligible() const { return

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

mizvekov updated this revision to Diff 335532. mizvekov added a comment. - small Doxygen fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99696/new/ https://reviews.llvm.org/D99696 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/Sema.c