[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

jyknight added a comment. I've gone ahead and created a revert review: https://reviews.llvm.org/D108243 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58514/new/ https://reviews.llvm.org/D58514 ___ cfe-commits mailing

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

jyknight added a comment. It appears as though this commit was reverted in Apple's XCode Clang fork -- the behavior currently in XCode matches the behavior of upstream Clang prior to this patch. Presuming that's correct, I think we should revert this upstream as well. There doesn't seem to be

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1435431 , @wuhao5 wrote: > In D58514#1435296 , @rjmccall wrote: > > > In D58514#1435228 , @wuhao5 wrote: > > > > > > Okay, so really just

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

wuhao5 added a comment. In D58514#1435296 , @rjmccall wrote: > In D58514#1435228 , @wuhao5 wrote: > > > > Okay, so really just a block self-reference. We could really just add a > > > feature for that that would

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1435228 , @wuhao5 wrote: > > Okay, so really just a block self-reference. We could really just add a > > feature for that that would avoid both the complexity and the expense of > > the self-capture dance. > > Is

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

wuhao5 added a comment. > Okay, so really just a block self-reference. We could really just add a > feature for that that would avoid both the complexity and the expense of the > self-capture dance. Is there a plan to cover this case? or is it a legitimate use case that Clang should

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1429758 , @wuhao5 wrote: > > Can I ask why you want a weak reference to a block in the first place? It > > seems basically useless — blocks can certainly appear in reference cycles, > > but I don't know why you'd

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

erik.pilkington added a comment. In D58514#1429713 , @rjmccall wrote: > In D58514#1429652 , @erik.pilkington > wrote: > > > In D58514#1429606 , @rjmccall > > wrote: > > >

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

wuhao5 added a comment. > Can I ask why you want a weak reference to a block in the first place? It > seems basically useless — blocks can certainly appear in reference cycles, > but I don't know why you'd ever try to break that cycle with the block > instead of somewhere else. The

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1429652 , @erik.pilkington wrote: > In D58514#1429606 , @rjmccall wrote: > > > There is no way in the existing ABI for copying a block to cause other > > references to the

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1429662 , @wuhao5 wrote: > > The specified user model of blocks is that they stay on the stack until > > they get copied, and there can be multiple such copies. ARC just automates > > that process. So the address of

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

wuhao5 added a comment. > The specified user model of blocks is that they stay on the stack until they > get copied, and there can be multiple such copies. ARC just automates that > process. So the address of a block is not consistent before you've forced a > copy. > > I tend to agree that

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

erik.pilkington added a comment. In D58514#1429606 , @rjmccall wrote: > There is no way in the existing ABI for copying a block to cause other > references to the block to become references to the heap block. We do do > that for `__block` variables,

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1429514 , @wuhao5 wrote: > Hey guys, this is Hao, I am working with Chrome team to sort this issue out. > > In D58514#1428606 , @rjmccall wrote: > > > I remember this coming up

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

wuhao5 added a comment. Hey guys, this is Hao, I am working with Chrome team to sort this issue out. In D58514#1428606 , @rjmccall wrote: > I remember this coming up 7-8 years ago. I intentionally decided against > doing a copy/release when assigning

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. I remember this coming up 7-8 years ago. I intentionally decided against doing a copy/release when assigning to `__weak` because if the block wasn't already guaranteed to be copied then it was probably better to crash than to silently assign a value that's about to

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

ahatanak added a comment. Do you mean copying the block to the heap before assigning it to `wb` and releasing it after the assignment inside `bar`? Wouldn't the block assigned to `wb` be deallocated after the release? Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

dexonsmith added a comment. In D58514#1428567 , @ahatanak wrote: > Sorry, I misread the code. If you change the parameter type of `bar` to > `BlockTy`, the code crashes. If the type is `id`, it doesn't crash because > IRGen copies the block to the heap

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

ahatanak added a comment. Sorry, I misread the code. If you change the parameter type of `bar` to `BlockTy`, the code crashes. If the type is `id`, it doesn't crash because IRGen copies the block to the heap in `foo` before passing it to `bar`. Repository: rC Clang CHANGES SINCE LAST

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

dexonsmith added a comment. In D58514#1428520 , @ahatanak wrote: > That code doesn't crash because the block is retained at the entry of `bar` > and ARC optimizer doesn't remove the retain/release pairs in `bar`. Oops, I meant: typedef void

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

ahatanak added a comment. In D58514#1428495 , @dexonsmith wrote: > In D58514#1428434 , @ahatanak wrote: > > > Seems like the chromium code is valid and shouldn't crash. John/Erik what > > do you think? The

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

dexonsmith added a comment. In D58514#1428434 , @ahatanak wrote: > Seems like the chromium code is valid and shouldn't crash. John/Erik what do > you think? The following code also crashes with this patch applied. > > typedef void (^BlockTy)(); > >

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

ahatanak added a comment. Seems like the chromium code is valid and shouldn't crash. John/Erik what do you think? The following code also crashes with this patch applied. typedef void (^BlockTy)(); BlockTy sb; __weak BlockTy wb; void foo(id a) { auto b = ^{ NSLog(@"foo %@",

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

thakis added a comment. Hi ahatanak, this causes a crash in chrome/ios. A reduced repro is at https://bugs.chromium.org/p/chromium/issues/detail?id=941680 . Is the code invalid, or is that a bug in the transform? Thanks, Nico Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

ahatanak closed this revision. ahatanak added a comment. Fixed in r355012. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58514/new/ https://reviews.llvm.org/D58514 ___ cfe-commits mailing list

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58514/new/ https://reviews.llvm.org/D58514 ___

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

ahatanak updated this revision to Diff 188472. ahatanak marked an inline comment as done. ahatanak added a comment. Call `IgnoreParens` on the LHS too. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58514/new/ https://reviews.llvm.org/D58514 Files:

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. Okay, one last minor request, then LGTM. Comment at: lib/CodeGen/CGObjC.cpp:2953 + return asImpl().visitExpr(e); +} + Oh, I'd forgotten this wasn't a normal expression visitor. Well, okay, this isn't too bad.

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

ahatanak updated this revision to Diff 187998. ahatanak marked 3 inline comments as done. ahatanak added a comment. Address review comments. Add CodeGen test cases for parentheses expressions. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58514/new/

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. I see, alright. Comment at: lib/CodeGen/CGObjC.cpp:3215 +if (be->getBlockDecl()->canAvoidCopyToHeap()) + return value; + Can this just be a case in `ARCRetainExprEmitter`? I think that should subsume both this and the logic

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

ahatanak updated this revision to Diff 187904. ahatanak added a comment. Remove the code that is needed to check whether the address of a local variable is taken. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58514/new/ https://reviews.llvm.org/D58514 Files:

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

ahatanak added a comment. For assignment, the optimization isn't performed if the local variable isn't declared in the scope that introduced the block (see the code and comment in SemaExpr.cpp). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58514/new/

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. The correctness condition here is solely that we cannot allow the block literal to go out of scope before the variable that it is assigned to. (Local block literals with captures have lifetimes like C compound literals: until the end of the enclosing block, rather

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

ahatanak created this revision. ahatanak added reviewers: rjmccall, erik.pilkington. ahatanak added a project: clang. Herald added subscribers: jdoerfert, dexonsmith, jkorous. This patch avoids copying blocks that initialize or are assigned to local auto variables to the heap when the local auto