[PATCH] D57797: Variable auto-init: fix __block initialization

[JF Bastien via Phabricator via cfe-commits]

jfb marked 3 inline comments as done.
jfb added inline comments.


================
Comment at: lib/CodeGen/CGDecl.cpp:1643
     CharUnits Size = getContext().getTypeSizeInChars(type);
     if (!Size.isZero()) {
       switch (trivialAutoVarInit) {
----------------
rjmccall wrote:
> jfb wrote:
> > rjmccall wrote:
> > > Can't you just drill to the byref storage right here and avoid all the other complexity and subtle ordering interactions?
> > We're in the lambda that does the initialization here. The tricky order part is that code that calls the lambda does:
> > 
> > - Block (which was missing the early auto-init)
> > - Trivial initializer (which has auto-init, then early exit)
> > - Constant aggregate / constexpr (which might auto-init if it wasn't constant, and then early-exit)
> > - The other stuff
> > 
> > I don't think here is the right place to do anything... and I'm not sure what you're suggesting.
> Escaping `__block` variables are basically a fixed-layout header followed by storage of the variable's formal type.  Anything you do at this point in the function to auto-initialize the header is a waste of time and code size because it is precisely at this point in the function that we perform a bunch of stores to initialize that header.   You are mitigating nothing by covering the header.  So what I'm saying is that, in this lambda which is meant to initialize the user-exposed storage of a variable, you should just make sure you're pointing at the user-exposed storage of the variable by calling `emitBlockByrefAddress(false)` (which just does a GEP), and then you can initialize only that.
Thanks, I now understand what you meant. It's super simple indeed, sorry I took a while to grok. Updated patch should do this now.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57797/new/

https://reviews.llvm.org/D57797