[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions

John McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 2 21:53:51 PDT 2018


rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1691
+
+    Name += "_" + llvm::to_string(E.Capture->getOffset().getQuantity());
+  }
----------------
I feel like this reads a little better if you write the quantity first.  Also I think you can drop the underscore because all of your suffices are easy to compute the length of, so a block that captures 3 strong variables can just be `32s40s48s` or something like that.

Oh, I guess if you do that you need to do something about how you handle the bit-mask for `BlockObject`, because that could run into the next number.  Would it be better to just break this out into cases?  The three current cases are (1) `__block` variables, which can throw, (2) blocks, and (3) object references in non-ARC modes (which for copy/dispose purposes are just `s` again, except we use a different entrypoint because we hadn't exposed `objc_retain` and `objc_release` yet).


================
Comment at: lib/CodeGen/CGBlocks.cpp:1647
+        unsigned Qs;
+        cast<CXXConstructExpr>(CE)->getConstructor()->isCopyConstructor(Qs);
+        if (Qs & Qualifiers::Volatile)
----------------
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > This doesn't always have to be a copy constructor.  I mean, maybe this procedure works anyway, but the constructor used to copy an object isn't always a copy constructor.  Blame constructor templates.
> > I've made changes to use the name of the constructor function that is used to copy the capture instead of using the volatility of the copy constructor's argument.
> > 
> > I'm not sure when a function that is not a copy constructor would be used to copy a captured object though. The comment in function captureInBlock in SemaExpr.cpp says that the the blocks spec requires a const copy constructor.
> Okay.  I think mangling the type of the capture is probably good enough (and likely to be a little smaller), because different compilations using the same capture type will always use the same constructor.  That will include `volatile` if the captured variable was marked `volatile`.
> 
> The notable case where copy-construction doesn't use a copy constructor is when you have a constructor template like `template <class T> ClassName(T &);`, which will be preferred over the default copy-constructor for copying non-`const` objects.  It's silly, but it's how it works.
> 
> You can mangle the type by just building an `ItaniumMangleContext`.  I think it's better to stably use the Itanium mangling instead of using the target's native mangling.
Itanium type manglings are self-delimiting, so you don't actually need this run-length encoding, but it doesn't really hurt, either.  Certainly it makes it a lot easier for tools to parse one of these function names.


================
Comment at: test/CodeGenCXX/block-byref-cxx-objc.cpp:36
+// CHECK: call void @_Block_object_dispose(
+
+int testB() {
----------------
ahatanak wrote:
> ahatanak wrote:
> > Should the second call to @_Block_object_dispose be an invoke if the destructor can throw? The FIXME in CodeGenFunction::BuildBlockRelease seems to imply that we should consider whether the destructor can throw.
> I mean the first call, not the second call. If the call to A's destructor throws when the first object is being destructed, the second object should be destructed on the EH path.
Yes, I think we should assume that the block runtime correctly propagates exceptions.  The function should not be marked `nothrow`, but call sites can be marked `nothrow` depending on what they do.

This won't generally be a code-size problem because (1) we do assume that the ObjC retain/release and weak operations can't throw and (2) C++11 makes destructors `noexcept` by default.


Repository:
  rC Clang

https://reviews.llvm.org/D50152





More information about the cfe-commits mailing list