Page MenuHomePhabricator

Distinguish `__block` variables that are captured by escaping blocks from those that aren't
ClosedPublic

Authored by ahatanak on Aug 31 2018, 6:11 PM.

Details

Summary

This patch changes the way __block variables that aren't captured by escaping blocks are handled:

  • Since non-escaping blocks on the stack never get copied to the heap (see https://reviews.llvm.org/D49303), Sema shouldn't error out when the type of a non-escaping __block variable doesn't have an accessible copy constructor.
  • IRGen doesn't have to use the specialized byref structure (see https://clang.llvm.org/docs/Block-ABI-Apple.html#id8) for a non-escaping __block variable anymore. Instead IRGen can emit the variable as a normal variable and copy the reference to the block literal . Byref copy/dispose helpers aren't needed either.

rdar://problem/39352313

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak created this revision.Aug 31 2018, 6:11 PM
rjmccall added inline comments.Aug 31 2018, 11:44 PM
include/clang/AST/Decl.h
977 ↗(On Diff #163613)

This doesn't actually seem to be initialized anywhere.

1422 ↗(On Diff #163613)

Documentation comments? I'm not opposed to using Byref as a shorthand here, but please remember that a lot of Clang maintainers aren't familiar with the blocks extension, and those that are aren't necessarily familiar with the non-escaping optimization.

3888 ↗(On Diff #163613)

Please call getVariable() here for clarity.

include/clang/Sema/ScopeInfo.h
438 ↗(On Diff #163613)

Please describe the expected operation in comments here: that blocks are added by default and then removed when the expression is provably used in a non-escaping way.

lib/CodeGen/CGBlocks.cpp
497 ↗(On Diff #163613)

Do you need to worry about the variable already having a reference type?

1310 ↗(On Diff #163613)

Isn't the field type already a reference type in this case?

You should leave a comment that you're relying on that, of course.

lib/Sema/Sema.cpp
1410 ↗(On Diff #163613)

It's okay to walk over an unordered set here because you're just changing memory. However, you don't actually need it to be a set; you can just remember all the blocks in the function and then ignore them if they're marked as non-escaping.

1412 ↗(On Diff #163613)

In contrast, it's not okay to walk over an unordered set here because the loop body can emit diagnostics and/or instantiate code.

Fortunately, you don't actually need ByrefBlockVars to be a set at all; you can just use an `llvm::TinyPtrVector'.

1414 ↗(On Diff #163613)

Please use an early continue so you can de-indent the rest of the loop body.

1420 ↗(On Diff #163613)

Should the contents of this block be in a helper function?

1451 ↗(On Diff #163613)

Why before?

ahatanak updated this revision to Diff 164069.Sep 5 2018, 10:30 AM
ahatanak marked 11 inline comments as done.

Address review comments. Fix bugs in the handling of __block variables that have reference types.

include/clang/AST/Decl.h
977 ↗(On Diff #163613)

VarDecl's constructor in Decl.cpp initializes it to false.

AllBits = 0;
VarDeclBits.SClass = SC;
// Everything else is implicitly initialized to false.
lib/CodeGen/CGBlocks.cpp
497 ↗(On Diff #163613)

Yes, I found out that the previous patch didn't handle __block variables with reference types correctly and caused assertion failures. To fix this, I made changes in computeBlockInfo so that the __block qualifier is ignored if the variable already has a reference type (I'm treating __block T& as T& ). There are no changes to this function.

1310 ↗(On Diff #163613)

I added an assertion that checks non-escaping variable fields have reference types.

lib/Sema/Sema.cpp
1451 ↗(On Diff #163613)

I remember I had to move the call to markEscapingByrefs to this place because it was making clang crash. It crashed when markEscapingByrefs triggered a call to PushFunctionScope, which caused clearing out PreallocatedFunctionScope, but I can't reproduce the crash anymore. I don't think markEscapingByrefs can cause PushFunctionScope to be called, so I'm moving the call back to the original location.

rjmccall accepted this revision.Sep 5 2018, 11:13 AM

LGTM.

include/clang/AST/Decl.h
977 ↗(On Diff #163613)

I see, thanks.

lib/CodeGen/CGBlocks.cpp
497 ↗(On Diff #163613)

Oh, I see. And that can be done unconditionally because of course the reference is never reseated. Makes sense.

This revision is now accepted and ready to land.Sep 5 2018, 11:13 AM
This revision was automatically updated to reflect the committed changes.