Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

[ahatanak]

Summary

This patch avoids copying blocks that initialize or are assigned to local auto variables to the heap when the local auto variables do not have their addresses taken and are declared in the same scope as the block. We can possibly add back the optimization in the ARC optimizer that was reverted in r189869 (http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130902/186509.html), but I suspect it would be much more complicated and fragile than doing it in the front-end.

I'm not 100% sure whether it's necessary to disable this optimization when the address of the local variable is taken. We do pass a block on the stack to a function when the block is directly passed instead of first being assigned to a local variable, but clang currently doesn't copy the passed block to the heap in the callee although the block can possibly escape if the address is taken. For example:

__strong id *g0, g1;

void foo0(BlockTy b) {
  g0 = (__strong id *)&b;
  g1 = *g0; // this is just a retain, not a block copy.
}

void foo1() {
  foo0(^{...}) // block is on the stack.
}

void foo2() {
  foo1();
  ((BlockTy)g1)(); // this can crash if the block is still on the stack.
}

rdar://problem/13289333

[rjmccall]

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 than the end of the full-expression.) So doing this blindly for assignments is problematic because you'd need to reason about relative lifetimes, but doing it for initialization should always be fine. I don't see any reason why address-taken-ness would matter as long as that condition holds.

[ahatanak]

Remove the code that is needed to check whether the address of a local variable is taken.

[ahatanak]

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).

[rjmccall]

I see, alright.

lib/CodeGen/CGObjC.cpp

3234

Can this just be a case in ARCRetainExprEmitter? I think that should subsume both this and the logic in EmitARCStoreStrong.

lib/Sema/SemaDecl.cpp

11262

I won't insist that you look through arbitrary value-propagating expressions like commas and conditionals, but please do at least call IgnoreParens() here and in the assignment case.

lib/Sema/SemaExpr.cpp

12485

Please check for a block-expression RHS first, it is far more likely to short-circuit this check than anything else.

Also, I think the right place for this check is up with the calls to DiagnoseSelfAssignment and DiagnoseSelfMove.

[ahatanak]

Address review comments. Add CodeGen test cases for parentheses expressions.

[rjmccall]

Okay, one last minor request, then LGTM.

lib/CodeGen/CGObjC.cpp

2953

Oh, I'd forgotten this wasn't a normal expression visitor. Well, okay, this isn't too bad.

lib/Sema/SemaExpr.cpp

12461

You should IgnoreParens on the LHS as well. In general, you should always IgnoreParens.

[ahatanak]

Call IgnoreParens on the LHS too.

[rjmccall]

Thanks, LGTM.

[ahatanak]

Fixed in r355012.

[thakis]

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

[ahatanak]

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 %@", a); };
  wb = b; // block isn't copied to the heap.
  sb = b; // block is copied to the heap.
}

int main() {
  auto x = [NSObject new];
  foo(x);
  sb();
  wb();
  return 0;
}

[dexonsmith]

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)();

BlockTy sb;
__weak BlockTy wb;

void foo(id a) {
  auto b = ^{ NSLog(@"foo %@", a); };
  wb = b; // block isn't copied to the heap.
  sb = b; // block is copied to the heap.
}

int main() {
  auto x = [NSObject new];
  foo(x);
  sb();
  wb();
  return 0;
}

The assignment to wb seems like an escape of some sort. What happens for this similar code?

typedef void (^BlockTy)();

BlockTy sb;
__weak BlockTy wb;

void bar(id b) {
  wb = b;
  sb = b;
}

void foo(id a) {
  bar(^{ NSLog(@"foo %@", a); });
}

int main() {
  auto x = [NSObject new];
  foo(x);
  sb();
  wb();
  return 0;
}

[ahatanak]

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 following code also crashes with this patch applied.

typedef void (^BlockTy)();

BlockTy sb;
__weak BlockTy wb;

void foo(id a) {
  auto b = ^{ NSLog(@"foo %@", a); };
  wb = b; // block isn't copied to the heap.
  sb = b; // block is copied to the heap.
}

int main() {
  auto x = [NSObject new];
  foo(x);
  sb();
  wb();
  return 0;
}

The assignment to wb seems like an escape of some sort. What happens for this similar code?

typedef void (^BlockTy)();

BlockTy sb;
__weak BlockTy wb;

void bar(id b) {
  wb = b;
  sb = b;
}

void foo(id a) {
  bar(^{ NSLog(@"foo %@", a); });
}

int main() {
  auto x = [NSObject new];
  foo(x);
  sb();
  wb();
  return 0;
}

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.

[dexonsmith]

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 (^BlockTy)();

BlockTy sb;
__weak BlockTy wb;

void bar(BlockTy b) {
  wb = b;
  sb = b;
}

void foo(id a) {
  bar(^{ NSLog(@"foo %@", a); });
}

int main() {
  auto x = [NSObject new];
  foo(x);
  sb();
  wb();
  return 0;
}

Is it the same? Does b get retained at the entry to bar()?

[ahatanak]

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.

[dexonsmith]

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 in foo before passing it to bar.

Okay, that's what I expected. (You didn't misread, I had just mistyped :/.)

How insane would it be to add a copy/dispose pair around assignments of blocks to weak references?

[ahatanak]

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?

[rjmccall]

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 be deallocated. Note that copying the block we assign into wb doesn't change anything about the block stored in b.

I don't know why Chromium is building a weak reference to a block in the first place, but assuming they have a good reason to be doing it, they should fix their code to force a copy before forming a weak reference.

[wuhao5]

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 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 be deallocated. Note that copying the block we assign into wb doesn't change anything about the block stored in b.

I don't know why Chromium is building a weak reference to a block in the first place, but assuming they have a good reason to be doing it, they should fix their code to force a copy before forming a weak reference.

The culprit code is here

it's true that it can be copied before assigning to the weak var, and I understand the reasoning behind. however, my question is: just from the code itself, each variable has the proper scope and assignment, if the block copy happen automatically, just like what we should expect ARC would do, should it not mutate itself to something else. to be more precise, should the block assigned to the weak var be the same after the block is copied? (and in the code, the block should be moved to the heap after calling -addObject: a few line below.)

so in the end of day, as a user, should we expect the compiler would move the block from stack to heap in time and the variable we hold is consistent?

[rjmccall]

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 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 be deallocated. Note that copying the block we assign into wb doesn't change anything about the block stored in b.

I don't know why Chromium is building a weak reference to a block in the first place, but assuming they have a good reason to be doing it, they should fix their code to force a copy before forming a weak reference.

The culprit code is here

it's true that it can be copied before assigning to the weak var, and I understand the reasoning behind. however, my question is: just from the code itself, each variable has the proper scope and assignment, if the block copy happen automatically, just like what we should expect ARC would do, should it not mutate itself to something else. to be more precise, should the block assigned to the weak var be the same after the block is copied? (and in the code, the block should be moved to the heap after calling -addObject: a few line below.)

so in the end of day, as a user, should we expect the compiler would move the block from stack to heap in time and the variable we hold is consistent?

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 a better user model would have been for blocks to be allocated in one place, without any of this copying business, and for the compiler to make an intelligent decision about stack vs. heap based on how the block is used. That's the model we've used for closures in Swift. But that would've required the compiler to have a better ability to propagate information about how the block was used, which Clang isn't really set up for, and it would've required noescape annotations to be introduced and used reliably throughout the SDK, which seemed like a big request at the time. So it's not how it works.

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, but not for block objects themselves.

[erik.pilkington]

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, but not for block objects themselves.

Do you think thats worth doing? We could add a forwarding pointer to the end of a block literal on the stack (after all the captures) and flip an unused bit to track it, preserving ABI. Seems like now that we're delaying _Block_copyies this might be a bigger issue.

[wuhao5]

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 a better user model would have been for blocks to be allocated in one place, without any of this copying business, and for the compiler to make an intelligent decision about stack vs. heap based on how the block is used. That's the model we've used for closures in Swift. But that would've required the compiler to have a better ability to propagate information about how the block was used, which Clang isn't really set up for, and it would've required noescape annotations to be introduced and used reliably throughout the SDK, which seemed like a big request at the time. So it's not how it works.

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, but not for block objects themselves.

I see - this makes sense. Right that I'd expect the compiler would know more about where the block is being used and make the variable consistent. my other worry is, although not realistically, that there can be other projects to use this weak/strong pointer trick to do a recursive block invocation. it becomes to me a bit counter-intuitive that I will need to know more about how block and where it should be copied, which currently we don't have to worry about it at all.

Right now we force an explicit copy before using it, but still like to request that this would be handled by Clang at some later point :)

[rjmccall]

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 a block is not consistent before you've forced a copy.

I tend to agree that a better user model would have been for blocks to be allocated in one place, without any of this copying business, and for the compiler to make an intelligent decision about stack vs. heap based on how the block is used. That's the model we've used for closures in Swift. But that would've required the compiler to have a better ability to propagate information about how the block was used, which Clang isn't really set up for, and it would've required noescape annotations to be introduced and used reliably throughout the SDK, which seemed like a big request at the time. So it's not how it works.

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, but not for block objects themselves.

I see - this makes sense. Right that I'd expect the compiler would know more about where the block is being used and make the variable consistent. my other worry is, although not realistically, that there can be other projects to use this weak/strong pointer trick to do a recursive block invocation. it becomes to me a bit counter-intuitive that I will need to know more about how block and where it should be copied, which currently we don't have to worry about it at all.

Right now we force an explicit copy before using it, but still like to request that this would be handled by Clang at some later point :)

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.

[rjmccall]

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 block to become references to the heap block. We do do that for __block variables, but not for block objects themselves.

Do you think thats worth doing? We could add a forwarding pointer to the end of a block literal on the stack (after all the captures) and flip an unused bit to track it, preserving ABI. Seems like now that we're delaying _Block_copyies this might be a bigger issue.

We've always delayed _Block_copy in a bunch of places. Now we're just delaying it in a place that ARC used to be more conservative about.

I guess we could actually make forwarding work at some code-size cost (functions that emitted forwardable blocks would have to zero the forwarding slot and release it when destroying the stack copy). But it'd just silently do nothing without a runtime update, so it'd be somewhat treacherous, especially after a couple of releases: e.g. if we made the runtime change in macOS 20 Eugene O'Neill National Historic Site, and Chromium eventually only ran tests on macOS 20 and higher but still supported deploying to macOS 19 San Francisco Maritime National Historical Park, then Chromium might not catch that it was still necessary to include an explicit copy here.

[wuhao5]

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 simplified version:

auto b = ^{
  if (check) {
    dispatch_after(queue, 1, b);
  } else {
   // done.
  }
};
dispatch_after(queue, 1, b);

[erik.pilkington]

In D58514#1429713, @rjmccall wrote:

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 block to become references to the heap block. We do do that for __block variables, but not for block objects themselves.

Do you think thats worth doing? We could add a forwarding pointer to the end of a block literal on the stack (after all the captures) and flip an unused bit to track it, preserving ABI. Seems like now that we're delaying _Block_copyies this might be a bigger issue.

We've always delayed _Block_copy in a bunch of places. Now we're just delaying it in a place that ARC used to be more conservative about.

I guess we could actually make forwarding work at some code-size cost (functions that emitted forwardable blocks would have to zero the forwarding slot and release it when destroying the stack copy). But it'd just silently do nothing without a runtime update, so it'd be somewhat treacherous, especially after a couple of releases: e.g. if we made the runtime change in macOS 20 Eugene O'Neill National Historic Site, and Chromium eventually only ran tests on macOS 20 and higher but still supported deploying to macOS 19 San Francisco Maritime National Historical Park, then Chromium might not catch that it was still necessary to include an explicit copy here.

Lol, we're really running out of names, eh? We could still do a version of this without the O'Neill/Maritime problem, to optimize the performance on the following:

void f(int (^blk)()) {
        // implicitly copy the block somehow...
}

int main() {
        int cap;
        auto p = ^{ return cap; };
        for (int i = 0; i != N; ++i)
                f(p);
}

Where we used to do 1 _Block_copy, but now we do N. If we stored a strong reference to the heap block in the stack block, then made _Block_copy just hand back the first heap block it allocated, we could save that cost. That way we could still preserve the crash here, then maybe once macOS Maritime is at EOL we could adopt the forwarding behaviour. WDYT?

[rjmccall]

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 ever try to break that cycle with the block instead of somewhere else.

The simplified version:

auto b = ^{

if (check) {
  dispatch_after(queue, 1, b);
} else {
 // done.
}

};
dispatch_after(queue, 1, b);

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.

[wuhao5]

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 handle?

[rjmccall]

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 there a plan to cover this case? or is it a legitimate use case that Clang should handle?

You are currently relying on something that ARC doesn't guarantee, so the client code should be fixed to explicitly copy the block. I think we would be happy to consider a proposal in the long run to allow blocks to self-reference more easily, which will effectively bypass the problem.

[wuhao5]

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 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 handle?

You are currently relying on something that ARC doesn't guarantee, so the client code should be fixed to explicitly copy the block. I think we would be happy to consider a proposal in the long run to allow blocks to self-reference more easily, which will effectively bypass the problem.

I am not sure if I follow here - is it not that the weak pointer holds a block that's in the stack but is supposed to be in the heap?

[rjmccall]

In D58514#1435431, @wuhao5 wrote:

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 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 handle?

You are currently relying on something that ARC doesn't guarantee, so the client code should be fixed to explicitly copy the block. I think we would be happy to consider a proposal in the long run to allow blocks to self-reference more easily, which will effectively bypass the problem.

I am not sure if I follow here - is it not that the weak pointer holds a block that's in the stack but is supposed to be in the heap?

In your code, ARC does not guarantee that the block pointer you're assigning to the weak reference will point to a heap copy of the block. ARC could force a copy of the block as part of the assignment, but it would be pointless because that copy would be immediately destroyed, leaving the weak reference holding nil, which is not what you want dynamically in your code. You need to force the block to be copied first.

[jyknight]

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 a good reason to have the semantics of blocks differ between Apple's clang and upstream clang.

[jyknight]

I've gone ahead and created a revert review: https://reviews.llvm.org/D108243

include/clang/AST/Decl.h

public:
 
   void setIsConversionFromLambda(bool val = true) {
     BlockDeclBits.IsConversionFromLambda = val;
   }
 
   bool doesNotEscape() const { return BlockDeclBits.DoesNotEscape; }
   void setDoesNotEscape(bool B = true) { BlockDeclBits.DoesNotEscape = B; }
 
+  bool canAvoidCopyToHeap() const {
+    return BlockDeclBits.CanAvoidCopyToHeap;
+  }
+  void setCanAvoidCopyToHeap(bool B = true) {
+    BlockDeclBits.CanAvoidCopyToHeap = B;
+  }
+
   bool capturesVariable(const VarDecl *var) const;
 
   void setCaptures(ASTContext &Context, ArrayRef<Capture> Captures,
                    bool CapturesCXXThis);
 
    unsigned getBlockManglingNumber() const {
      return ManglingNumber;
    }

include/clang/AST/DeclBase.h

class BlockDeclBitfields {
     uint64_t IsVariadic : 1;
     uint64_t CapturesCXXThis : 1;
     uint64_t BlockMissingReturnType : 1;
     uint64_t IsConversionFromLambda : 1;
 
     /// A bit that indicates this block is passed directly to a function as a
     /// non-escaping parameter.
     uint64_t DoesNotEscape : 1;
+
+    /// A bit that indicates whether it's possible to avoid coying this block to
+    /// the heap when it initializes or is assigned to a local variable with
+    /// automatic storage.
+    uint64_t CanAvoidCopyToHeap : 1;
   };
 
   /// Number of non-inherited bits in BlockDeclBitfields.
   enum { NumBlockDeclBits = 5 };
 
   /// Pointer to the data structure used to lookup declarations
   /// within this context (or a DependentStoredDeclsMap if this is a
   /// dependent context). We maintain the invariant that, if the map

lib/AST/Decl.cpp

 
 BlockDecl::BlockDecl(DeclContext *DC, SourceLocation CaretLoc)
     : Decl(Block, DC, CaretLoc), DeclContext(Block) {
   setIsVariadic(false);
   setCapturesCXXThis(false);
   setBlockMissingReturnType(true);
   setIsConversionFromLambda(false);
   setDoesNotEscape(false);
+  setCanAvoidCopyToHeap(false);
 }
 
 void BlockDecl::setParams(ArrayRef<ParmVarDecl *> NewParamInfo) {
   assert(!ParamInfo && "Already has param info!");
 
   // Zero params -> null pointer.
   if (!NewParamInfo.empty()) {
     NumParams = NewParamInfo.size();

lib/CodeGen/CGObjC.cpp

protected:
   Impl &asImpl() { return *static_cast<Impl*>(this); }
 
   ARCExprEmitter(CodeGenFunction &CGF) : CGF(CGF) {}
 
 public:
   Result visit(const Expr *e);
   Result visitCastExpr(const CastExpr *e);
   Result visitPseudoObjectExpr(const PseudoObjectExpr *e);
+  Result visitBlockExpr(const BlockExpr *e);
   Result visitBinaryOperator(const BinaryOperator *e);
   Result visitBinAssign(const BinaryOperator *e);
   Result visitBinAssignUnsafeUnretained(const BinaryOperator *e);
   Result visitBinAssignAutoreleasing(const BinaryOperator *e);
   Result visitBinAssignWeak(const BinaryOperator *e);
   Result visitBinAssignStrong(const BinaryOperator *e);
 
   // Minimal implementation:
ARCExprEmitter<Impl,Result>::visitPseudoObjectExpr(const PseudoObjectExpr *E) {
   // Unbind all the opaques now.
   for (unsigned i = 0, e = opaques.size(); i != e; ++i)
     opaques[i].unbind(CGF);
 
   return result;
 }
 
 template <typename Impl, typename Result>
+Result ARCExprEmitter<Impl, Result>::visitBlockExpr(const BlockExpr *e) {
+  // The default implementation just forwards the expression to visitExpr.
+  return asImpl().visitExpr(e);
+}
+
+template <typename Impl, typename Result>
 Result ARCExprEmitter<Impl,Result>::visitCastExpr(const CastExpr *e) {
   switch (e->getCastKind()) {
 
   // No-op casts don't change the type, so we just ignore them.
   case CK_NoOp:
     return asImpl().visit(e->getSubExpr());
 
   // These casts can change the type.
Result ARCExprEmitter<Impl,Result>::visit(const Expr *e) {
   } else if (isa<CallExpr>(e) ||
              (isa<ObjCMessageExpr>(e) &&
               !cast<ObjCMessageExpr>(e)->isDelegateInitCall())) {
     return asImpl().visitCall(e);
 
   // Look through pseudo-object expressions.
   } else if (const PseudoObjectExpr *pseudo = dyn_cast<PseudoObjectExpr>(e)) {
     return asImpl().visitPseudoObjectExpr(pseudo);
-  }
+  } else if (auto *be = dyn_cast<BlockExpr>(e))
+    return asImpl().visitBlockExpr(be);
 
   return asImpl().visitExpr(e);
 }
 
 namespace {
 
 /// An emitter for +1 results.
 struct ARCRetainExprEmitter :
struct ARCRetainExprEmitter :
 
   /// For consumptions, just emit the subexpression and thus elide
   /// the retain/release pair.
   TryEmitResult visitConsumeObject(const Expr *e) {
     llvm::Value *result = CGF.EmitScalarExpr(e);
     return TryEmitResult(result, true);
   }
 
+  TryEmitResult visitBlockExpr(const BlockExpr *e) {
+    TryEmitResult result = visitExpr(e);
+    // Avoid the block-retain if this is a block literal that doesn't need to be
+    // copied to the heap.
+    if (e->getBlockDecl()->canAvoidCopyToHeap())
+      result.setInt(true);
+    return result;
+  }
+
   /// Block extends are net +0.  Naively, we could just recurse on
   /// the subexpression, but actually we need to ensure that the
   /// value is copied as a block, so there's a little filter here.
   TryEmitResult visitExtendBlockObject(const Expr *e) {
     llvm::Value *result; // will be a +0 value
 
     // If we can't safely assume the sub-expression will produce a
     // block-copied value, emit the sub-expression at +0.
if (!result.getInt())
     value = EmitARCRetain(e->getType(), value);
   return value;
 }
 
 llvm::Value *
 CodeGenFunction::EmitARCRetainAutoreleaseScalarExpr(const Expr *e) {
   // The retain needs to happen within the full-expression.
   if (const ExprWithCleanups *cleanups = dyn_cast<ExprWithCleanups>(e)) {
     enterFullExpression(cleanups);
     RunCleanupsScope scope(*this);
     return EmitARCRetainAutoreleaseScalarExpr(cleanups->getSubExpr());
   }
 
   TryEmitResult result = tryEmitARCRetainScalarExpr(*this, e);
   llvm::Value *value = result.getPointer();
   if (result.getInt())
     value = EmitARCAutorelease(value);

lib/Sema/SemaDecl.cpp

// constant expressions.
                isa<InitListExpr>(Init)) {
       const Expr *Culprit;
       if (!Init->isConstantInitializer(Context, false, &Culprit)) {
         Diag(Culprit->getExprLoc(),
              diag::ext_aggregate_init_not_constant)
           << Culprit->getSourceRange();
       }
     }
+
+    if (auto *E = dyn_cast<ExprWithCleanups>(Init))
+      if (auto *BE = dyn_cast<BlockExpr>(E->getSubExpr()->IgnoreParens()))
+        if (VDecl->hasLocalStorage())
+          BE->getBlockDecl()->setCanAvoidCopyToHeap();
   } else if (VDecl->isStaticDataMember() && !VDecl->isInline() &&
              VDecl->getLexicalDeclContext()->isRecord()) {
     // This is an in-class initialization for a static data member, e.g.,
     //
     // struct S {
     //   static const int value = 17;
     // };
 

lib/Sema/SemaExpr.cpp

case BO_Assign:
     if (getLangOpts().CPlusPlus &&
         LHS.get()->getObjectKind() != OK_ObjCProperty) {
       VK = LHS.get()->getValueKind();
       OK = LHS.get()->getObjectKind();
     }
     if (!ResultTy.isNull()) {
       DiagnoseSelfAssignment(*this, LHS.get(), RHS.get(), OpLoc, true);
       DiagnoseSelfMove(LHS.get(), RHS.get(), OpLoc);
+
+      // Avoid copying a block to the heap if the block is assigned to a local
+      // auto variable that is declared in the same scope as the block. This
+      // optimization is unsafe if the local variable is declared in an outer
+      // scope. For example:
+      //
+      // BlockTy b;
+      // {
+      //   b = ^{...};
+      // }
+      // // It is unsafe to invoke the block here if it wasn't copied to the
+      // // heap.
+      // b();
+
+      if (auto *BE = dyn_cast<BlockExpr>(RHS.get()->IgnoreParens()))
+        if (auto *DRE = dyn_cast<DeclRefExpr>(LHS.get()->IgnoreParens()))
+          if (auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
+            if (VD->hasLocalStorage() && getCurScope()->isDeclScope(VD))
+              BE->getBlockDecl()->setCanAvoidCopyToHeap();
     }
     RecordModifiableNonNullParam(*this, LHS.get());
     break;
   case BO_PtrMemD:
   case BO_PtrMemI:
     ResultTy = CheckPointerToMemberOperands(LHS, RHS, VK, OpLoc,
                                             Opc == BO_PtrMemI);
     break;
   case BO_Mul:
   case BO_Div:
     ConvertHalfVec = true;
     ResultTy = CheckMultiplyDivideOperands(LHS, RHS, OpLoc, false,
                                            Opc == BO_Div);
     break;
   case BO_Rem:
     ResultTy = CheckRemainderOperands(LHS, RHS, OpLoc);
     break;
   case BO_Add:
     ConvertHalfVec = true;
     ResultTy = CheckAdditionOperands(LHS, RHS, OpLoc, Opc);
     break;
   case BO_Sub:
     ConvertHalfVec = true;
     ResultTy = CheckSubtractionOperands(LHS, RHS, OpLoc);
     break;
   case BO_Shl:
   case BO_Shr:
     ResultTy = CheckShiftOperands(LHS, RHS, OpLoc, Opc);
     break;

lib/Serialization/ASTReaderDecl.cpp

void ASTDeclReader::VisitBlockDecl(BlockDecl *BD) {
   for (unsigned I = 0; I != NumParams; ++I)
     Params.push_back(ReadDeclAs<ParmVarDecl>());
   BD->setParams(Params);
 
   BD->setIsVariadic(Record.readInt());
   BD->setBlockMissingReturnType(Record.readInt());
   BD->setIsConversionFromLambda(Record.readInt());
   BD->setDoesNotEscape(Record.readInt());
+  BD->setCanAvoidCopyToHeap(Record.readInt());
 
   bool capturesCXXThis = Record.readInt();
   unsigned numCaptures = Record.readInt();
   SmallVector<BlockDecl::Capture, 16> captures;
   captures.reserve(numCaptures);
   for (unsigned i = 0; i != numCaptures; ++i) {
     auto *decl = ReadDeclAs<VarDecl>();
     unsigned flags = Record.readInt();

lib/Serialization/ASTWriterDecl.cpp

void ASTDeclWriter::VisitBlockDecl(BlockDecl *D) {
   Record.AddTypeSourceInfo(D->getSignatureAsWritten());
   Record.push_back(D->param_size());
   for (ParmVarDecl *P : D->parameters())
     Record.AddDeclRef(P);
   Record.push_back(D->isVariadic());
   Record.push_back(D->blockMissingReturnType());
   Record.push_back(D->isConversionFromLambda());
   Record.push_back(D->doesNotEscape());
+  Record.push_back(D->canAvoidCopyToHeap());
   Record.push_back(D->capturesCXXThis());
   Record.push_back(D->getNumCaptures());
   for (const auto &capture : D->captures()) {
     Record.AddDeclRef(capture.getVariable());
 
     unsigned flags = 0;
     if (capture.isByRef()) flags |= 1;
     if (capture.isNested()) flags |= 2;

test/CodeGenObjC/arc-block-copy-escape.m

 // RUN: %clang_cc1 -fobjc-arc -fblocks -emit-llvm %s -o - | FileCheck %s
 
 typedef void (^block_t)(void);
 void use_block(block_t);
 void use_int(int);
 
 // rdar://problem/10211676
 
 void test0(int i) {
   block_t block = ^{ use_int(i); };
   // CHECK-LABEL:   define {{.*}}void @test0(
-  // CHECK:     call {{.*}}i8* @llvm.objc.retainBlock(i8* {{%.*}}) [[NUW:#[0-9]+]], !clang.arc.copy_on_escape
+  // CHECK-NOT: @llvm.objc.retainBlock(
   // CHECK:     ret void
 }
 
 void test1(int i) {
   id block = ^{ use_int(i); };
   // CHECK-LABEL:   define {{.*}}void @test1(
-  // CHECK:     call {{.*}}i8* @llvm.objc.retainBlock(i8* {{%.*}}) [[NUW]]
+  // CHECK:     call {{.*}}i8* @llvm.objc.retainBlock(i8* {{%.*}}) [[NUW:#[0-9]+]]
   // CHECK-NOT: !clang.arc.copy_on_escape
   // CHECK:     ret void
 }
 
 // CHECK: attributes [[NUW]] = { nounwind }

test/CodeGenObjC/arc-blocks.m

 
 // rdar://problem/9814099
 // Test that we correctly initialize __block variables
 // when the initialization captures the variable.
 void test10a(void) {
   __block void (^block)(void) = ^{ block(); };
   // CHECK-LABEL:    define void @test10a()
   // CHECK:      [[BYREF:%.*]] = alloca [[BYREF_T:%.*]],
+  // CHECK:      [[BLOCK1:%.*]] = alloca <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, align 8
 
   // Zero-initialization before running the initializer.
   // CHECK:      [[T0:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[BYREF]], i32 0, i32 6
   // CHECK-NEXT: store void ()* null, void ()** [[T0]], align 8
 
   // Run the initializer as an assignment.
-  // CHECK:      [[T0:%.*]] = bitcast void ()* {{%.*}} to i8*
-  // CHECK-NEXT: [[T1:%.*]] = call i8* @llvm.objc.retainBlock(i8* [[T0]])
-  // CHECK-NEXT: [[T2:%.*]] = bitcast i8* [[T1]] to void ()*
+  // CHECK:      [[T2:%.*]] = bitcast <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* [[BLOCK1]] to void ()*
   // CHECK-NEXT: [[T3:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[BYREF]], i32 0, i32 1
   // CHECK-NEXT: [[T4:%.*]] = load [[BYREF_T]]*, [[BYREF_T]]** [[T3]]
   // CHECK-NEXT: [[T5:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[T4]], i32 0, i32 6
   // CHECK-NEXT: [[T6:%.*]] = load void ()*, void ()** [[T5]], align 8
-  // CHECK-NEXT: store void ()* {{%.*}}, void ()** [[T5]], align 8
+  // CHECK-NEXT: store void ()* [[T2]], void ()** [[T5]], align 8
   // CHECK-NEXT: [[T7:%.*]] = bitcast void ()* [[T6]] to i8*
   // CHECK-NEXT: call void @llvm.objc.release(i8* [[T7]])
 
   // Destroy at end of function.
   // CHECK-NEXT: [[SLOT:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[BYREF]], i32 0, i32 6
   // CHECK-NEXT: [[T0:%.*]] = bitcast [[BYREF_T]]* [[BYREF]] to i8*
   // CHECK-NEXT: call void @_Block_object_dispose(i8* [[T0]], i32 8)
   // CHECK-NEXT: [[T1:%.*]] = load void ()*, void ()** [[SLOT]]

 // Test that we correctly assign to __block variables when the
 // assignment captures the variable.
 void test10b(void) {
   __block void (^block)(void);
   block = ^{ block(); };
 
   // CHECK-LABEL:    define void @test10b()
   // CHECK:      [[BYREF:%.*]] = alloca [[BYREF_T:%.*]],
+  // CHECK:      [[BLOCK3:%.*]] = alloca <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, align 8
 
   // Zero-initialize.
   // CHECK:      [[T0:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[BYREF]], i32 0, i32 6
   // CHECK-NEXT: store void ()* null, void ()** [[T0]], align 8
 
   // CHECK-NEXT: [[SLOT:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[BYREF]], i32 0, i32 6
 
   // The assignment.
-  // CHECK:      [[T0:%.*]] = bitcast void ()* {{%.*}} to i8*
-  // CHECK-NEXT: [[T1:%.*]] = call i8* @llvm.objc.retainBlock(i8* [[T0]])
-  // CHECK-NEXT: [[T2:%.*]] = bitcast i8* [[T1]] to void ()*
+  // CHECK:      [[T2:%.*]] = bitcast <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* [[BLOCK3]] to void ()*
   // CHECK-NEXT: [[T3:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[BYREF]], i32 0, i32 1
   // CHECK-NEXT: [[T4:%.*]] = load [[BYREF_T]]*, [[BYREF_T]]** [[T3]]
   // CHECK-NEXT: [[T5:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[T4]], i32 0, i32 6
   // CHECK-NEXT: [[T6:%.*]] = load void ()*, void ()** [[T5]], align 8
-  // CHECK-NEXT: store void ()* {{%.*}}, void ()** [[T5]], align 8
+  // CHECK-NEXT: store void ()* [[T2]], void ()** [[T5]], align 8
   // CHECK-NEXT: [[T7:%.*]] = bitcast void ()* [[T6]] to i8*
   // CHECK-NEXT: call void @llvm.objc.release(i8* [[T7]])
 
   // Destroy at end of function.
   // CHECK-NEXT: [[T0:%.*]] = bitcast [[BYREF_T]]* [[BYREF]] to i8*
   // CHECK-NEXT: call void @_Block_object_dispose(i8* [[T0]], i32 8)
   // CHECK-NEXT: [[T1:%.*]] = load void ()*, void ()** [[SLOT]]
   // CHECK-NEXT: [[T2:%.*]] = bitcast void ()* [[T1]] to i8*

test/CodeGenObjCXX/arc-blocks.mm

void foo1() {
   S0 t3, t4;
   // Capturing a non-external type doesn't cause the copy/dispose helpers to be
   // internal unless the captured type has a non-trivial copy constructor or
   // destructor.
   TrivialInternal t5;
   ^{ (void)t0; (void)t1; (void)t2; (void)t3; (void)t4; (void)t5; };
 }
 }
+
+// Test that calls to @llvm.objc.retainBlock aren't emitted in some cases.
+
+namespace test_block_retain {
+  typedef void (^BlockTy)();
+
+  void foo1(id);
+
+// CHECK-LABEL: define void @_ZN17test_block_retain14initializationEP11objc_object(
+// CHECK-NOT: @llvm.objc.retainBlock(
+  void initialization(id a) {
+    BlockTy b0 = ^{ foo1(a); };
+    BlockTy b1 = (^{ foo1(a); });
+    b0();
+    b1();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain20initializationStaticEP11objc_object(
+// CHECK: @llvm.objc.retainBlock(
+  void initializationStatic(id a) {
+    static BlockTy b0 = ^{ foo1(a); };
+    b0();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain15initialization2EP11objc_object
+// CHECK: %[[B0:.*]] = alloca void ()*, align 8
+// CHECK: %[[B1:.*]] = alloca void ()*, align 8
+// CHECK: load void ()*, void ()** %[[B0]], align 8
+// CHECK-NOT: @llvm.objc.retainBlock
+// CHECK: %[[V9:.*]] = load void ()*, void ()** %[[B0]], align 8
+// CHECK: %[[V10:.*]] = bitcast void ()* %[[V9]] to i8*
+// CHECK: %[[V11:.*]] = call i8* @llvm.objc.retainBlock(i8* %[[V10]])
+// CHECK: %[[V12:.*]] = bitcast i8* %[[V11]] to void ()*
+// CHECK: store void ()* %[[V12]], void ()** %[[B1]], align 8
+  void initialization2(id a) {
+    BlockTy b0 = ^{ foo1(a); };
+    b0();
+    BlockTy b1 = b0; // can't optimize this yet.
+    b1();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain10assignmentEP11objc_object(
+// CHECK-NOT: @llvm.objc.retainBlock(
+  void assignment(id a) {
+    BlockTy b0;
+    (b0) = ^{ foo1(a); };
+    b0();
+    b0 = (^{ foo1(a); });
+    b0();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain16assignmentStaticEP11objc_object(
+// CHECK: @llvm.objc.retainBlock(
+  void assignmentStatic(id a) {
+    static BlockTy b0;
+    b0 = ^{ foo1(a); };
+    b0();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain21assignmentConditionalEP11objc_objectb(
+// CHECK: @llvm.objc.retainBlock(
+  void assignmentConditional(id a, bool c) {
+    BlockTy b0;
+    if (c)
+      // can't optimize this since 'b0' is declared in the outer scope.
+      b0 = ^{ foo1(a); };
+    b0();
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain11assignment2EP11objc_object(
+// CHECK: %[[B0:.*]] = alloca void ()*, align 8
+// CHECK: %[[B1:.*]] = alloca void ()*, align 8
+// CHECK-NOT: @llvm.objc.retainBlock
+// CHECK: store void ()* null, void ()** %[[B1]], align 8
+// CHECK: %[[V9:.*]] = load void ()*, void ()** %[[B0]], align 8
+// CHECK: %[[V10:.*]] = bitcast void ()* %[[V9]] to i8*
+// CHECK: %[[V11:.*]] = call i8* @llvm.objc.retainBlock(i8* %[[V10]]
+// CHECK: %[[V12:.*]] = bitcast i8* %[[V11]] to void ()*
+// CHECK: store void ()* %[[V12]], void ()** %[[B1]], align 8
+  void assignment2(id a) {
+    BlockTy b0 = ^{ foo1(a); };
+    b0();
+    BlockTy b1;
+    b1 = b0; // can't optimize this yet.
+    b1();
+  }
+
+// We cannot remove the call to @llvm.objc.retainBlock if the variable is of type id.
+
+// CHECK: define void @_ZN17test_block_retain21initializationObjCPtrEP11objc_object(
+// CHECK: alloca i8*, align 8
+// CHECK: %[[B0:.*]] = alloca i8*, align 8
+// CHECK: %[[BLOCK:.*]] = alloca <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, align 8
+// CHECK: %[[V3:.*]] = bitcast <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]] to void ()*
+// CHECK: %[[V4:.*]] = bitcast void ()* %[[V3]] to i8*
+// CHECK: %[[V5:.*]] = call i8* @llvm.objc.retainBlock(i8* %[[V4]])
+// CHECK: %[[V6:.*]] = bitcast i8* %[[V5]] to void ()*
+// CHECK: %[[V7:.*]] = bitcast void ()* %[[V6]] to i8*
+// CHECK: store i8* %[[V7]], i8** %[[B0]], align 8
+  void initializationObjCPtr(id a) {
+    id b0 = ^{ foo1(a); };
+    ((BlockTy)b0)();
+  }
+
+// CHECK: define void @_ZN17test_block_retain17assignmentObjCPtrEP11objc_object(
+// CHECK: %[[B0:.*]] = alloca void ()*, align 8
+// CHECK: %[[B1:.*]] = alloca i8*, align 8
+// CHECK: %[[V4:.*]] = load void ()*, void ()** %[[B0]], align 8
+// CHECK: %[[V5:.*]] = bitcast void ()* %[[V4]] to i8*
+// CHECK: %[[V6:.*]] = call i8* @llvm.objc.retainBlock(i8* %[[V5]])
+// CHECK: %[[V7:.*]] = bitcast i8* %[[V6]] to void ()*
+// CHECK: %[[V8:.*]] = bitcast void ()* %[[V7]] to i8*
+// CHECK: store i8* %[[V8]], i8** %[[B1]], align 8
+  void assignmentObjCPtr(id a) {
+    BlockTy b0 = ^{ foo1(a); };
+    id b1;
+    b1 = b0;
+    ((BlockTy)b1)();
+  }
+}

test/PCH/arc-blocks.mm

+// RUN: %clang_cc1 -fobjc-arc -fblocks -std=c++1y -emit-pch %s -o %t
+// RUN: %clang_cc1 -fobjc-arc -fblocks -std=c++1y -include-pch %t -emit-llvm -o - %s | FileCheck %s
+
+#ifndef HEADER_INCLUDED
+#define HEADER_INCLUDED
+
+namespace test_block_retain {
+  typedef void (^BlockTy)();
+  void foo1(id);
+
+  inline void initialization(id a) {
+    // Call to @llvm.objc.retainBlock isn't needed.
+    BlockTy b0 = ^{ foo1(a); };
+    b0();
+  }
+
+  inline void assignmentConditional(id a, bool c) {
+    BlockTy b0;
+    if (c)
+      // @llvm.objc.retainBlock is called since 'b0' is declared in the outer scope.
+      b0 = ^{ foo1(a); };
+    b0();
+  }
+}
+
+#else
+
+// CHECK: %[[STRUCT_BLOCK_DESCRIPTOR:.*]] = type { i64, i64 }
+
+namespace test_block_retain {
+// CHECK-LABEL: define linkonce_odr void @_ZN17test_block_retain14initializationEP11objc_object(
+// CHECK-NOT: call i8* @llvm.objc.retainBlock(
+
+  void test_initialization(id a) {
+    initialization(a);
+  }
+
+// CHECK-LABEL: define void @_ZN17test_block_retain26test_assignmentConditionalEP11objc_objectb(
+// CHECK: %[[BLOCK:.*]] = alloca <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, align 8
+// CHECK: %[[V4:.*]] = bitcast <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]] to void ()*
+// CHECK: %[[V5:.*]] = bitcast void ()* %[[V4]] to i8*
+// CHECK: call i8* @llvm.objc.retainBlock(i8* %[[V5]])
+
+  void test_assignmentConditional(id a, bool c) {
+    assignmentConditional(a, c);
+  }
+}
+
+#endif