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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
