This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen][ObjC] Treat non-escaping blocks as global blocks to make copy/dispose a no-op
ClosedPublic

Authored by ahatanak on Jul 13 2018, 9:36 AM.

Details

Summary

Copying or disposing of a non-escaping block can be a no-op. A non-escaping block on the stack doesn't have to be copied to the heap as we know it won't be called after its lifetime ends.

rdar://problem/39352313

Diff Detail

Repository
rC Clang

Event Timeline

ahatanak created this revision.Jul 13 2018, 9:36 AM

Please test that we still copy captures correctly into the block object and destroy them when the block object is destroyed.

For the sorts of ephemeral blocks that we currently mark non-escaping, we can think about optimizing that away, too, but for now please test that we still perform captures normally.

ahatanak planned changes to this revision.Jul 18 2018, 4:12 PM

Please test that we still copy captures correctly into the block object and destroy them when the block object is destroyed.

There is a bug in my patch where the capture cleanups aren't pushed when doesNotEscape() returns true. I'm working on a fix.

ahatanak updated this revision to Diff 156332.Jul 19 2018, 11:50 AM

Fix a bug where the capture cleanups weren't pushed when BlockDecl::doesNotEscape() returns true.

Test that, when ARC is enabled, captures are retained and copied into the stack block object and destroyed when the stack block object goes out of scope.

rjmccall added inline comments.Jul 19 2018, 12:24 PM
docs/Block-ABI-Apple.rst
64

Something happened to my older comments here, but please document the meaning of this flag. Specifically, it is set on blocks that do have captures (and thus are not true global blocks) but which are known not to escape for various other reasons. Please include the fact that it implies BLOCK_IS_GLOBAL and explain why.

lib/CodeGen/CGBlocks.h
283

Please rename this to needsCopyDisposeHelpers() to make it clearer that it's specifically about the need for the functions.

ahatanak updated this revision to Diff 156404.Jul 19 2018, 5:15 PM
ahatanak marked 2 inline comments as done.

Add a comment that explains the meaning of BLOCK_IS_NOESCAPE to the docs. Rename function needsCopyDispose to needsCopyDisposeHelpers.

rjmccall accepted this revision.Jul 19 2018, 6:28 PM

Thanks. A couple tiny things and then LGTM.

docs/Block-ABI-Apple.rst
69

Thanks, that looks good. I think it's important to emphasize that setting BLOCK_IS_GLOBAL is necessary *for backward compatibility*, though.

You have a newline between the comment and BLOCK_IS_NOESCAPE that feels out-of-place.

This revision is now accepted and ready to land.Jul 19 2018, 6:28 PM
This revision was automatically updated to reflect the committed changes.
ahatanak marked an inline comment as done.

@ahatanak @rjmccall What if a block is marked with noescape and captures some variables at the same time? Wouldn't that cause problem If the compiler continues to optimize it to NSConcreteGlobal type anyway?

Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 6:23 AM

Blocks that don't capture anything have always been allocated globally. This optimization is using the global ISA for blocks that do capture locals but which are known statically (on penalty of UB) to not escape the original scope of the block literal. Using the global ISA causes attempts to copy these blocks to have no effect, in case the program attempts to do unnecessary copies despite the block being declared non-escaping.