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

[JF Bastien via Phabricator via cfe-commits]

jfb marked an inline comment as done.
jfb added inline comments.


================
Comment at: lib/CodeGen/CGDecl.cpp:1642
 
     CharUnits Size = getContext().getTypeSizeInChars(type);
     if (!Size.isZero()) {
----------------
jfb wrote:
> rjmccall wrote:
> > Does this check handle flexible arrays on zero-sized structures properly?  I suppose they're always initialized.
> You mean something like
> ```
> void foo(int size) {
>   struct Z {};
>   Z arr[size];
> }
> ```
> ?
Just to circle back, the above code lowers to:

```
%struct.Z = type { i8 }

@__const._Z3fooi.arr = private unnamed_addr constant %struct.Z { i8 -86 }, align 1

; Function Attrs: noinline nounwind optnone ssp uwtable
define void @_Z3fooi(i32 %size) #0 {
entry:
  %size.addr = alloca i32, align 4
  %saved_stack = alloca i8*, align 8
  %__vla_expr0 = alloca i64, align 8
  store i32 %size, i32* %size.addr, align 4
  %0 = load i32, i32* %size.addr, align 4
  %1 = zext i32 %0 to i64
  %2 = call i8* @llvm.stacksave()
  store i8* %2, i8** %saved_stack, align 8
  %vla = alloca %struct.Z, i64 %1, align 16
  store i64 %1, i64* %__vla_expr0, align 8
  %vla.iszerosized = icmp eq i64 %1, 0
  br i1 %vla.iszerosized, label %vla-init.cont, label %vla-setup.loop

vla-setup.loop:                                   ; preds = %entry
  %vla.begin = bitcast %struct.Z* %vla to i8*
  %vla.end = getelementptr inbounds i8, i8* %vla.begin, i64 %1
  br label %vla-init.loop

vla-init.loop:                                    ; preds = %vla-init.loop, %vla-setup.loop
  %vla.cur = phi i8* [ %vla.begin, %vla-setup.loop ], [ %vla.next, %vla-init.loop ]
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %vla.cur, i8* align 1 getelementptr inbounds (%struct.Z, %struct.Z* @__const._Z3fooi.arr, i32 0, i32 0), i64 1, i1 false)
  %vla.next = getelementptr inbounds i8, i8* %vla.cur, i64 1
  %vla-init.isdone = icmp eq i8* %vla.next, %vla.end
  br i1 %vla-init.isdone, label %vla-init.cont, label %vla-init.loop

vla-init.cont:                                    ; preds = %vla-init.loop, %entry
  %3 = load i8*, i8** %saved_stack, align 8
  call void @llvm.stackrestore(i8* %3)
  ret void
}
```

So yes it's handled.


================
Comment at: lib/CodeGen/CGDecl.cpp:1663
     const VariableArrayType *VlaType =
         dyn_cast_or_null<VariableArrayType>(getContext().getAsArrayType(type));
     if (!VlaType)
----------------
jfb wrote:
> rjmccall wrote:
> > This is a late comment on the main patch, but there's an `ASTContext::getAsVariableArrayType` method.
> OK I can do in a separate follow-up.
https://reviews.llvm.org/rC353490


================
Comment at: lib/CodeGen/CGDecl.cpp:1605
-    emitByrefStructureInit(emission);
-
   // Initialize the variable here if it doesn't have a initializer and it is a
----------------
jfb wrote:
> rjmccall wrote:
> > Are these changes still needed?
> I believe so, see concurrent comment here:
> https://reviews.llvm.org/D57797#inline-512702
Actually I'm wrong, updated patch drops this and the test makes sure the call is after this initialization. Sorry for the confusion. This is much cleaner.


================
Comment at: lib/CodeGen/CGDecl.cpp:1726
+    emitByrefStructureInit(emission);
+  }
+
----------------
rjmccall wrote:
> jfb wrote:
> > Note that we still want this to be pulled out in this way because `emitByrefStructureInit` emits the call to the initializer (in `test_block_self_init` the call to `create`). Were we to leave this as it was before, one of the initializations below would be emitted, but it would be *after* the call.
> > 
> > Similarly, we also want the little dance I added to only initialize once.
> Oh, that's interesting, I hadn't remembered that.  Alright, in that case LGTM, I guess.
> 
> ...unless you want to refactor this so that `emitByrefStructureInit` only handles the header initialization and the bit about handling the initializer is handled more cleanly down in the rest of this code.
As noted above, I was wrong here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57797