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

[ahatanak]

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

[rjmccall]

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]

In D49303#1165902, @rjmccall wrote:

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]

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]

docs/Block-ABI-Apple.rst

64 ↗

(On Diff #156332)

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 ↗

(On Diff #156332)

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

[ahatanak]

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

[rjmccall]

Thanks. A couple tiny things and then LGTM.

docs/Block-ABI-Apple.rst

69 ↗

(On Diff #156404)

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.

[jajadude]

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

[rjmccall]

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.

cfe/trunk/docs/Block-ABI-Apple.rst

struct Block_literal_1 {
         // imported variables
     };
 
 The following flags bits are in use thusly for a possible ABI.2010.3.16:
 
 .. code-block:: c
 
     enum {
+        // Set to true on blocks that have captures (and thus are not true
+        // global blocks) but are known not to escape for various other
+        // reasons. For backward compatiblity with old runtimes, whenever
+        // BLOCK_IS_NOESCAPE is set, BLOCK_IS_GLOBAL is set too. Copying a
+        // non-escaping block returns the original block and releasing such a
+        // block is a no-op, which is exactly how global blocks are handled.
+        BLOCK_IS_NOESCAPE      =  (1 << 23),
+
         BLOCK_HAS_COPY_DISPOSE =  (1 << 25),
         BLOCK_HAS_CTOR =          (1 << 26), // helpers have C++ code
         BLOCK_IS_GLOBAL =         (1 << 28),
         BLOCK_HAS_STRET =         (1 << 29), // IFF BLOCK_HAS_SIGNATURE
         BLOCK_HAS_SIGNATURE =     (1 << 30), 
     };
 
 In 10.6.ABI the (1<<29) was usually set and was always ignored by the runtime -

cfe/trunk/lib/CodeGen/CGBlocks.h

enum BlockByrefFlags {
   BLOCK_BYREF_LAYOUT_EXTENDED          = (1   << 28),
   BLOCK_BYREF_LAYOUT_NON_OBJECT        = (2   << 28),
   BLOCK_BYREF_LAYOUT_STRONG            = (3   << 28),
   BLOCK_BYREF_LAYOUT_WEAK              = (4   << 28),
   BLOCK_BYREF_LAYOUT_UNRETAINED        = (5   << 28)
 };
 
 enum BlockLiteralFlags {
+  BLOCK_IS_NOESCAPE      =  (1 << 23),
   BLOCK_HAS_COPY_DISPOSE =  (1 << 25),
   BLOCK_HAS_CXX_OBJ =       (1 << 26),
   BLOCK_IS_GLOBAL =         (1 << 28),
   BLOCK_USE_STRET =         (1 << 29),
   BLOCK_HAS_SIGNATURE  =    (1 << 30),
   BLOCK_HAS_EXTENDED_LAYOUT = (1 << 31)
 };
 class BlockFlags {
static Capture makeConstant(llvm::Value *value) {
       return v;
     }    
   };
 
   /// CanBeGlobal - True if the block can be global, i.e. it has
   /// no non-constant captures.
   bool CanBeGlobal : 1;
 
-  /// True if the block needs a custom copy or dispose function.
+  /// True if the block has captures that would necessitate custom copy or
+  /// dispose helper functions if the block were escaping.
   bool NeedsCopyDispose : 1;
 
   /// HasCXXObject - True if the block's custom copy/dispose functions
   /// need to be run even in GC mode.
   bool HasCXXObject : 1;
 
   /// UsesStret : True if the block uses an stret return.  Mutable
   /// because it gets set later in the block-creation process.
public:
   const BlockDecl *getBlockDecl() const { return Block; }
   const BlockExpr *getBlockExpr() const {
     assert(BlockExpression);
     assert(BlockExpression->getBlockDecl() == Block);
     return BlockExpression;
   }
 
   CGBlockInfo(const BlockDecl *blockDecl, StringRef Name);
+
+  // Indicates whether the block needs a custom copy or dispose function.
+  bool needsCopyDisposeHelpers() const {
+    return NeedsCopyDispose && !Block->doesNotEscape();
+  }
 };
 
 }  // end namespace CodeGen
 }  // end namespace clang
 
 #endif

cfe/trunk/lib/CodeGen/CGBlocks.cpp

static llvm::Constant *buildBlockDescriptor(CodeGenModule &CGM,
 
   // Size
   // FIXME: What is the right way to say this doesn't fit?  We should give
   // a user diagnostic in that case.  Better fix would be to change the
   // API to size_t.
   elements.addInt(ulong, blockInfo.BlockSize.getQuantity());
 
   // Optional copy/dispose helpers.
-  if (blockInfo.NeedsCopyDispose) {
+  if (blockInfo.needsCopyDisposeHelpers()) {
     // copy_func_helper_decl
     elements.add(buildCopyHelper(CGM, blockInfo));
 
     // destroy_func_decl
     elements.add(buildDisposeHelper(CGM, blockInfo));
   }
 
   // Signature.  Mandatory ObjC-style method descriptor @encode sequence.
struct Block_literal {
     /// In theory, we could start one off malloc'ed by setting
     /// BLOCK_NEEDS_FREE, giving it a refcount of 1, and using
     /// this isa:
     ///   extern void *_NSConcreteMallocBlock[];
     struct objc_class *isa;
 
     /// These are the flags (with corresponding bit number) that the
     /// compiler is actually supposed to know about.
+    ///  23. BLOCK_IS_NOESCAPE - indicates that the block is non-escaping
     ///  25. BLOCK_HAS_COPY_DISPOSE - indicates that the block
     ///   descriptor provides copy and dispose helper functions
     ///  26. BLOCK_HAS_CXX_OBJ - indicates that there's a captured
     ///   object with a nontrivial destructor or copy constructor
     ///  28. BLOCK_IS_GLOBAL - indicates that the block is allocated
     ///   as global memory
     ///  29. BLOCK_USE_STRET - indicates that the block function
     ///   uses stret, which objc_msgSend needs to know about
llvm::Value *CodeGenFunction::EmitBlockLiteral(const CGBlockInfo &blockInfo) {
 
   Address blockAddr = blockInfo.LocalAddress;
   assert(blockAddr.isValid() && "block has no address!");
 
   llvm::Constant *isa;
   llvm::Constant *descriptor;
   BlockFlags flags;
   if (!IsOpenCL) {
-    isa = llvm::ConstantExpr::getBitCast(CGM.getNSConcreteStackBlock(),
-                                         VoidPtrTy);
+    // If the block is non-escaping, set field 'isa 'to NSConcreteGlobalBlock
+    // and set the BLOCK_IS_GLOBAL bit of field 'flags'. Copying a non-escaping
+    // block just returns the original block and releasing it is a no-op.
+    llvm::Constant *blockISA = blockInfo.getBlockDecl()->doesNotEscape()
+                                   ? CGM.getNSConcreteGlobalBlock()
+                                   : CGM.getNSConcreteStackBlock();
+    isa = llvm::ConstantExpr::getBitCast(blockISA, VoidPtrTy);
 
     // Build the block descriptor.
     descriptor = buildBlockDescriptor(CGM, blockInfo);
 
     // Compute the initial on-stack block flags.
     flags = BLOCK_HAS_SIGNATURE;
     if (blockInfo.HasCapturedVariableLayout)
       flags |= BLOCK_HAS_EXTENDED_LAYOUT;
-    if (blockInfo.NeedsCopyDispose)
+    if (blockInfo.needsCopyDisposeHelpers())
       flags |= BLOCK_HAS_COPY_DISPOSE;
     if (blockInfo.HasCXXObject)
       flags |= BLOCK_HAS_CXX_OBJ;
     if (blockInfo.UsesStret)
       flags |= BLOCK_USE_STRET;
+    if (blockInfo.getBlockDecl()->doesNotEscape())
+      flags |= BLOCK_IS_NOESCAPE | BLOCK_IS_GLOBAL;
   }
 
   auto projectField =
     [&](unsigned index, CharUnits offset, const Twine &name) -> Address {
       return Builder.CreateStructGEP(blockAddr, index, offset, name);
     };
   auto storeField =
     [&](llvm::Value *value, unsigned index, CharUnits offset,

cfe/trunk/test/CodeGenObjC/noescape.m

-// RUN: %clang_cc1 -triple x86_64-apple-darwin -fblocks -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fblocks -emit-llvm -o - %s | FileCheck -check-prefix CHECK -check-prefix CHECK-NOARC %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fblocks -emit-llvm -fobjc-arc -o - %s | FileCheck -check-prefix CHECK -check-prefix CHECK-ARC %s
 
 typedef void (^BlockTy)(void);
 
 union U {
   int *i;
   long long *ll;
 } __attribute__((transparent_union));
 
 void noescapeFunc0(id, __attribute__((noescape)) BlockTy);
 void noescapeFunc1(__attribute__((noescape)) int *);
 void noescapeFunc2(__attribute__((noescape)) id);
 void noescapeFunc3(__attribute__((noescape)) union U);
 
+// Block descriptors of non-escaping blocks don't need pointers to copy/dispose
+// helper functions.
+
+// CHECK: %[[STRUCT_BLOCK_DESCRIPTOR:.*]] = type { i64, i64 }
+// CHECK: @[[BLOCK_DESCIPTOR_TMP_2:.*]] = internal constant { i64, i64, i8*, i64 } { i64 0, i64 40, i8* getelementptr inbounds ([6 x i8], [6 x i8]* @{{.*}}, i32 0, i32 0), i64 256 }, align 8
+
 // CHECK-LABEL: define void @test0(
 // CHECK: call void @noescapeFunc0({{.*}}, {{.*}} nocapture {{.*}})
 // CHECK: declare void @noescapeFunc0(i8*, {{.*}} nocapture)
 void test0(BlockTy b) {
   noescapeFunc0(0, b);
 }
 
 // CHECK-LABEL: define void @test1(

 // CHECK: define internal void @{{.*}}(i8* {{.*}}, i32* nocapture {{.*}})
 
 typedef void (^BlockTy2)(__attribute__((noescape)) int *);
 
 void test5(BlockTy2 b, int *p) {
   ^(int *__attribute__((noescape)) p0){}(p);
   b(p);
 }
+
+// If the block is non-escaping, set the BLOCK_IS_NOESCAPE and BLOCK_IS_GLOBAL
+// bits of field 'flags' and set the 'isa' field to 'NSConcreteGlobalBlock'.
+
+// CHECK: define void @test6(i8* %{{.*}}, i8* %[[B:.*]])
+// CHECK: %{{.*}} = alloca i8*, align 8
+// CHECK: %[[B_ADDR:.*]] = alloca i8*, align 8
+// CHECK: %[[BLOCK:.*]] = alloca <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, align 8
+// CHECK-NOARC: store i8* %[[B]], i8** %[[B_ADDR]], align 8
+// CHECK-ARC: store i8* null, i8** %[[B_ADDR]], align 8
+// CHECK-ARC: call void @objc_storeStrong(i8** %[[B_ADDR]], i8* %[[B]])
+// CHECK-ARC: %[[V0:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]], i32 0, i32 5
+// CHECK: %[[BLOCK_ISA:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]], i32 0, i32 0
+// CHECK: store i8* bitcast (i8** @_NSConcreteGlobalBlock to i8*), i8** %[[BLOCK_ISA]], align 8
+// CHECK: %[[BLOCK_FLAGS:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]], i32 0, i32 1
+// CHECK: store i32 -796917760, i32* %[[BLOCK_FLAGS]], align 8
+// CHECK: %[[BLOCK_DESCRIPTOR:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]], i32 0, i32 4
+// CHECK: store %[[STRUCT_BLOCK_DESCRIPTOR]]* bitcast ({ i64, i64, i8*, i64 }* @[[BLOCK_DESCIPTOR_TMP_2]] to %[[STRUCT_BLOCK_DESCRIPTOR]]*), %[[STRUCT_BLOCK_DESCRIPTOR]]** %[[BLOCK_DESCRIPTOR]], align 8
+// CHECK: %[[BLOCK_CAPTURED:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]], i32 0, i32 5
+// CHECK-NOARC: %[[V1:.*]] = load i8*, i8** %[[B_ADDR]], align 8
+// CHECK-NOARC: store i8* %[[V1]], i8** %[[BLOCK_CAPTURED]], align 8
+// CHECK-ARC: %[[V2:.*]] = load i8*, i8** %[[B_ADDR]], align 8
+// CHECK-ARC: %[[V3:.*]] = call i8* @objc_retain(i8* %[[V2]]) #3
+// CHECK-ARC: store i8* %[[V3]], i8** %[[BLOCK_CAPTURED]], align 8
+// CHECK: call void @noescapeFunc0(
+// CHECK-ARC: call void @objc_storeStrong(i8** %[[V0]], i8* null)
+// CHECK-ARC: call void @objc_storeStrong(i8** %[[B_ADDR]], i8* null)
+
+// Non-escaping blocks don't need copy/dispose helper functions.
+
+// CHECK-NOT: define internal void @__copy_helper_block_
+// CHECK-NOT: define internal void @__destroy_helper_block_
+
+void func(id);
+
+void test6(id a, id b) {
+  noescapeFunc0(a, ^{ func(b); });
+}