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

2019-02-07 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment.

SGTM, thanks!


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


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

2019-02-07 Thread JF Bastien via Phabricator via cfe-commits
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rC353495: Variable auto-init: fix __block initialization 
(authored by jfb, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57797?vs=185896=185902#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57797

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGenCXX/trivial-auto-var-init.cpp


Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1633,11 +1633,15 @@
   ? LangOptions::TrivialAutoVarInitKind::Uninitialized
   : getContext().getLangOpts().getTrivialAutoVarInit()));
 
-  auto initializeWhatIsTechnicallyUninitialized = [&]() {
+  auto initializeWhatIsTechnicallyUninitialized = [&](Address Loc) {
 if (trivialAutoVarInit ==
 LangOptions::TrivialAutoVarInitKind::Uninitialized)
   return;
 
+// Only initialize a __block's storage: we always initialize the header.
+if (emission.IsEscapingByRef)
+  Loc = emitBlockByrefAddress(Loc, , /*follow=*/false);
+
 CharUnits Size = getContext().getTypeSizeInChars(type);
 if (!Size.isZero()) {
   switch (trivialAutoVarInit) {
@@ -1714,7 +1718,7 @@
   };
 
   if (isTrivialInitializer(Init)) {
-initializeWhatIsTechnicallyUninitialized();
+initializeWhatIsTechnicallyUninitialized(Loc);
 return;
   }
 
@@ -1728,7 +1732,7 @@
   }
 
   if (!constant) {
-initializeWhatIsTechnicallyUninitialized();
+initializeWhatIsTechnicallyUninitialized(Loc);
 LValue lv = MakeAddrLValue(Loc, type);
 lv.setNonGC(true);
 return EmitExprAsInit(Init, , lv, capturedByInit);
Index: test/CodeGenCXX/trivial-auto-var-init.cpp
===
--- test/CodeGenCXX/trivial-auto-var-init.cpp
+++ test/CodeGenCXX/trivial-auto-var-init.cpp
@@ -30,6 +30,32 @@
   used(block);
 }
 
+// Using the variable being initialized is typically UB in C, but for blocks we
+// can be nice: they imply extra book-keeping and we can do the auto-init 
before
+// any of said book-keeping.
+//
+// UNINIT-LABEL:  test_block_self_init(
+// ZERO-LABEL:test_block_self_init(
+// ZERO:  %block = alloca <{ i8*, i32, i32, i8*, 
%struct.__block_descriptor*, i8* }>, align 8
+// ZERO:  %captured1 = getelementptr inbounds 
%struct.__block_byref_captured, %struct.__block_byref_captured* %captured, i32 
0, i32 4
+// ZERO-NEXT: store %struct.XYZ* null, %struct.XYZ** %captured1, align 8
+// ZERO:  %call = call %struct.XYZ* @create(
+// PATTERN-LABEL: test_block_self_init(
+// PATTERN:   %block = alloca <{ i8*, i32, i32, i8*, 
%struct.__block_descriptor*, i8* }>, align 8
+// PATTERN:   %captured1 = getelementptr inbounds 
%struct.__block_byref_captured, %struct.__block_byref_captured* %captured, i32 
0, i32 4
+// PATTERN-NEXT:  store %struct.XYZ* inttoptr (i64 -6148914691236517206 to 
%struct.XYZ*), %struct.XYZ** %captured1, align 8
+// PATTERN:   %call = call %struct.XYZ* @create(
+void test_block_self_init() {
+  using Block = void (^)();
+  typedef struct XYZ {
+Block block;
+  } * xyz_t;
+  extern xyz_t create(Block block);
+  __block xyz_t captured = create(^() {
+(void)captured;
+  });
+}
+
 // This type of code is currently not handled by zero / pattern initialization.
 // The test will break when that is fixed.
 // UNINIT-LABEL:  test_goto_unreachable_value(


Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1633,11 +1633,15 @@
   ? LangOptions::TrivialAutoVarInitKind::Uninitialized
   : getContext().getLangOpts().getTrivialAutoVarInit()));
 
-  auto initializeWhatIsTechnicallyUninitialized = [&]() {
+  auto initializeWhatIsTechnicallyUninitialized = [&](Address Loc) {
 if (trivialAutoVarInit ==
 LangOptions::TrivialAutoVarInitKind::Uninitialized)
   return;
 
+// Only initialize a __block's storage: we always initialize the header.
+if (emission.IsEscapingByRef)
+  Loc = emitBlockByrefAddress(Loc, , /*follow=*/false);
+
 CharUnits Size = getContext().getTypeSizeInChars(type);
 if (!Size.isZero()) {
   switch (trivialAutoVarInit) {
@@ -1714,7 +1718,7 @@
   };
 
   if (isTrivialInitializer(Init)) {
-initializeWhatIsTechnicallyUninitialized();
+initializeWhatIsTechnicallyUninitialized(Loc);
 return;
   }
 
@@ -1728,7 +1732,7 @@
   }
 
   if (!constant) {
-initializeWhatIsTechnicallyUninitialized();
+initializeWhatIsTechnicallyUninitialized(Loc);
 LValue lv = MakeAddrLValue(Loc, type);
 lv.setNonGC(true);
 return EmitExprAsInit(Init, , lv, capturedByInit);
Index: 

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

2019-02-07 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 185896.
jfb added a comment.

- Remove whitespace changes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57797

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGenCXX/trivial-auto-var-init.cpp


Index: test/CodeGenCXX/trivial-auto-var-init.cpp
===
--- test/CodeGenCXX/trivial-auto-var-init.cpp
+++ test/CodeGenCXX/trivial-auto-var-init.cpp
@@ -30,6 +30,32 @@
   used(block);
 }
 
+// Using the variable being initialized is typically UB in C, but for blocks we
+// can be nice: they imply extra book-keeping and we can do the auto-init 
before
+// any of said book-keeping.
+//
+// UNINIT-LABEL:  test_block_self_init(
+// ZERO-LABEL:test_block_self_init(
+// ZERO:  %block = alloca <{ i8*, i32, i32, i8*, 
%struct.__block_descriptor*, i8* }>, align 8
+// ZERO:  %captured1 = getelementptr inbounds 
%struct.__block_byref_captured, %struct.__block_byref_captured* %captured, i32 
0, i32 4
+// ZERO-NEXT: store %struct.XYZ* null, %struct.XYZ** %captured1, align 8
+// ZERO:  %call = call %struct.XYZ* @create(
+// PATTERN-LABEL: test_block_self_init(
+// PATTERN:   %block = alloca <{ i8*, i32, i32, i8*, 
%struct.__block_descriptor*, i8* }>, align 8
+// PATTERN:   %captured1 = getelementptr inbounds 
%struct.__block_byref_captured, %struct.__block_byref_captured* %captured, i32 
0, i32 4
+// PATTERN-NEXT:  store %struct.XYZ* inttoptr (i64 -6148914691236517206 to 
%struct.XYZ*), %struct.XYZ** %captured1, align 8
+// PATTERN:   %call = call %struct.XYZ* @create(
+void test_block_self_init() {
+  using Block = void (^)();
+  typedef struct XYZ {
+Block block;
+  } * xyz_t;
+  extern xyz_t create(Block block);
+  __block xyz_t captured = create(^() {
+(void)captured;
+  });
+}
+
 // This type of code is currently not handled by zero / pattern initialization.
 // The test will break when that is fixed.
 // UNINIT-LABEL:  test_goto_unreachable_value(
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1633,11 +1633,15 @@
   ? LangOptions::TrivialAutoVarInitKind::Uninitialized
   : getContext().getLangOpts().getTrivialAutoVarInit()));
 
-  auto initializeWhatIsTechnicallyUninitialized = [&]() {
+  auto initializeWhatIsTechnicallyUninitialized = [&](Address Loc) {
 if (trivialAutoVarInit ==
 LangOptions::TrivialAutoVarInitKind::Uninitialized)
   return;
 
+// Only initialize a __block's storage: we always initialize the header.
+if (emission.IsEscapingByRef)
+  Loc = emitBlockByrefAddress(Loc, , /*follow=*/false);
+
 CharUnits Size = getContext().getTypeSizeInChars(type);
 if (!Size.isZero()) {
   switch (trivialAutoVarInit) {
@@ -1714,7 +1718,7 @@
   };
 
   if (isTrivialInitializer(Init)) {
-initializeWhatIsTechnicallyUninitialized();
+initializeWhatIsTechnicallyUninitialized(Loc);
 return;
   }
 
@@ -1728,7 +1732,7 @@
   }
 
   if (!constant) {
-initializeWhatIsTechnicallyUninitialized();
+initializeWhatIsTechnicallyUninitialized(Loc);
 LValue lv = MakeAddrLValue(Loc, type);
 lv.setNonGC(true);
 return EmitExprAsInit(Init, , lv, capturedByInit);


Index: test/CodeGenCXX/trivial-auto-var-init.cpp
===
--- test/CodeGenCXX/trivial-auto-var-init.cpp
+++ test/CodeGenCXX/trivial-auto-var-init.cpp
@@ -30,6 +30,32 @@
   used(block);
 }
 
+// Using the variable being initialized is typically UB in C, but for blocks we
+// can be nice: they imply extra book-keeping and we can do the auto-init before
+// any of said book-keeping.
+//
+// UNINIT-LABEL:  test_block_self_init(
+// ZERO-LABEL:test_block_self_init(
+// ZERO:  %block = alloca <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i8* }>, align 8
+// ZERO:  %captured1 = getelementptr inbounds %struct.__block_byref_captured, %struct.__block_byref_captured* %captured, i32 0, i32 4
+// ZERO-NEXT: store %struct.XYZ* null, %struct.XYZ** %captured1, align 8
+// ZERO:  %call = call %struct.XYZ* @create(
+// PATTERN-LABEL: test_block_self_init(
+// PATTERN:   %block = alloca <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i8* }>, align 8
+// PATTERN:   %captured1 = getelementptr inbounds %struct.__block_byref_captured, %struct.__block_byref_captured* %captured, i32 0, i32 4
+// PATTERN-NEXT:  store %struct.XYZ* inttoptr (i64 -6148914691236517206 to %struct.XYZ*), %struct.XYZ** %captured1, align 8
+// PATTERN:   %call = call %struct.XYZ* @create(
+void test_block_self_init() {
+  using Block = void (^)();
+  typedef struct XYZ {
+Block block;
+  } * xyz_t;
+  extern xyz_t create(Block block);
+  __block xyz_t captured = create(^() {
+(void)captured;
+  });

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

2019-02-07 Thread 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(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


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

2019-02-07 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 185892.
jfb marked 4 inline comments as done.
jfb added a comment.

- Simplify patch greatly.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57797

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGenCXX/trivial-auto-var-init.cpp


Index: test/CodeGenCXX/trivial-auto-var-init.cpp
===
--- test/CodeGenCXX/trivial-auto-var-init.cpp
+++ test/CodeGenCXX/trivial-auto-var-init.cpp
@@ -30,6 +30,32 @@
   used(block);
 }
 
+// Using the variable being initialized is typically UB in C, but for blocks we
+// can be nice: they imply extra book-keeping and we can do the auto-init 
before
+// any of said book-keeping.
+//
+// UNINIT-LABEL:  test_block_self_init(
+// ZERO-LABEL:test_block_self_init(
+// ZERO:  %block = alloca <{ i8*, i32, i32, i8*, 
%struct.__block_descriptor*, i8* }>, align 8
+// ZERO:  %captured1 = getelementptr inbounds 
%struct.__block_byref_captured, %struct.__block_byref_captured* %captured, i32 
0, i32 4
+// ZERO-NEXT: store %struct.XYZ* null, %struct.XYZ** %captured1, align 8
+// ZERO:  %call = call %struct.XYZ* @create(
+// PATTERN-LABEL: test_block_self_init(
+// PATTERN:   %block = alloca <{ i8*, i32, i32, i8*, 
%struct.__block_descriptor*, i8* }>, align 8
+// PATTERN:   %captured1 = getelementptr inbounds 
%struct.__block_byref_captured, %struct.__block_byref_captured* %captured, i32 
0, i32 4
+// PATTERN-NEXT:  store %struct.XYZ* inttoptr (i64 -6148914691236517206 to 
%struct.XYZ*), %struct.XYZ** %captured1, align 8
+// PATTERN:   %call = call %struct.XYZ* @create(
+void test_block_self_init() {
+  using Block = void (^)();
+  typedef struct XYZ {
+Block block;
+  } * xyz_t;
+  extern xyz_t create(Block block);
+  __block xyz_t captured = create(^() {
+(void)captured;
+  });
+}
+
 // This type of code is currently not handled by zero / pattern initialization.
 // The test will break when that is fixed.
 // UNINIT-LABEL:  test_goto_unreachable_value(
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1598,7 +1598,7 @@
 if (!Init || !ContainsLabel(Init)) return;
 EnsureInsertPoint();
   }
-
+  
   // Initialize the structure of a __block variable.
   if (emission.IsEscapingByRef)
 emitByrefStructureInit(emission);
@@ -1606,9 +1606,8 @@
   // Initialize the variable here if it doesn't have a initializer and it is a
   // C struct that is non-trivial to initialize or an array containing such a
   // struct.
-  if (!Init &&
-  type.isNonTrivialToPrimitiveDefaultInitialize() ==
-  QualType::PDIK_Struct) {
+  if (!Init && type.isNonTrivialToPrimitiveDefaultInitialize() ==
+   QualType::PDIK_Struct) {
 LValue Dst = MakeAddrLValue(emission.getAllocatedAddress(), type);
 if (emission.IsEscapingByRef)
   drillIntoBlockVariable(*this, Dst, );
@@ -1633,11 +1632,15 @@
   ? LangOptions::TrivialAutoVarInitKind::Uninitialized
   : getContext().getLangOpts().getTrivialAutoVarInit()));
 
-  auto initializeWhatIsTechnicallyUninitialized = [&]() {
+  auto initializeWhatIsTechnicallyUninitialized = [&](Address Loc) {
 if (trivialAutoVarInit ==
 LangOptions::TrivialAutoVarInitKind::Uninitialized)
   return;
 
+// Only initialize a __block's storage: we always initialize the header.
+if (emission.IsEscapingByRef)
+  Loc = emitBlockByrefAddress(Loc, , /*follow=*/false);
+
 CharUnits Size = getContext().getTypeSizeInChars(type);
 if (!Size.isZero()) {
   switch (trivialAutoVarInit) {
@@ -1715,7 +1718,7 @@
   };
 
   if (isTrivialInitializer(Init)) {
-initializeWhatIsTechnicallyUninitialized();
+initializeWhatIsTechnicallyUninitialized(Loc);
 return;
   }
 
@@ -1729,7 +1732,7 @@
   }
 
   if (!constant) {
-initializeWhatIsTechnicallyUninitialized();
+initializeWhatIsTechnicallyUninitialized(Loc);
 LValue lv = MakeAddrLValue(Loc, type);
 lv.setNonGC(true);
 return EmitExprAsInit(Init, , lv, capturedByInit);


Index: test/CodeGenCXX/trivial-auto-var-init.cpp
===
--- test/CodeGenCXX/trivial-auto-var-init.cpp
+++ test/CodeGenCXX/trivial-auto-var-init.cpp
@@ -30,6 +30,32 @@
   used(block);
 }
 
+// Using the variable being initialized is typically UB in C, but for blocks we
+// can be nice: they imply extra book-keeping and we can do the auto-init before
+// any of said book-keeping.
+//
+// UNINIT-LABEL:  test_block_self_init(
+// ZERO-LABEL:test_block_self_init(
+// ZERO:  %block = alloca <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i8* }>, align 8
+// ZERO:  %captured1 = getelementptr inbounds %struct.__block_byref_captured, %struct.__block_byref_captured* 

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

2019-02-07 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:1726
+emitByrefStructureInit(emission);
+  }
+

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.


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


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

2019-02-07 Thread JF Bastien via Phabricator via cfe-commits
jfb marked 2 inline comments as done.
jfb added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:1605
-emitByrefStructureInit(emission);
-
   // Initialize the variable here if it doesn't have a initializer and it is a

rjmccall wrote:
> Are these changes still needed?
I believe so, see concurrent comment here:
https://reviews.llvm.org/D57797#inline-512702


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


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

2019-02-07 Thread JF Bastien via Phabricator via cfe-commits
jfb marked 2 inline comments as done.
jfb added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:1726
+emitByrefStructureInit(emission);
+  }
+

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.


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


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

2019-02-07 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:1605
-emitByrefStructureInit(emission);
-
   // Initialize the variable here if it doesn't have a initializer and it is a

Are these changes still needed?



Comment at: lib/CodeGen/CGDecl.cpp:1644
+if (emission.IsEscapingByRef)
+  Loc = emitBlockByrefAddress(Loc, , /*follow=*/false);
 

Thanks, this is what I was looking for.


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


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

2019-02-07 Thread JF Bastien via Phabricator via cfe-commits
jfb updated this revision to Diff 185883.
jfb added a comment.

- Only initialize __block's storage.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57797

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGenCXX/trivial-auto-var-init.cpp
  test/CodeGenObjC/arc-blocks.m
  test/CodeGenObjC/blocks-2.m
  test/CodeGenObjC/blocks.m
  test/CodeGenObjCXX/arc-blocks.mm

Index: test/CodeGenObjCXX/arc-blocks.mm
===
--- test/CodeGenObjCXX/arc-blocks.mm
+++ test/CodeGenObjCXX/arc-blocks.mm
@@ -24,14 +24,14 @@
   }
   // CHECK-LABEL:define void @_ZN5test03fooEv() 
   // CHECK:  [[V:%.*]] = alloca [[BYREF_A:%.*]], align 8
+  // CHECK:  [[V1:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 7
   // CHECK:  [[T0:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 4
   // CHECK-NEXT: store i8* bitcast (void (i8*, i8*)* [[COPY_HELPER:@.*]] to i8*), i8** [[T0]]
   // CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 5
   // CHECK-NEXT: store i8* bitcast (void (i8*)* [[DISPOSE_HELPER:@.*]] to i8*), i8** [[T0]]
   // CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 6
   // CHECK-NEXT: store i8* getelementptr inbounds ([3 x i8], [3 x i8]* [[LAYOUT0]], i32 0, i32 0), i8** [[T0]]
-  // CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 7
-  // CHECK-NEXT: call void @_ZN5test01AC1Ev([[A]]* [[T0]])
+  // CHECK-NEXT: call void @_ZN5test01AC1Ev([[A]]* [[V1]])
   // CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 7
   // CHECK: bitcast [[BYREF_A]]* [[V]] to i8*
   // CHECK: [[T1:%.*]] = bitcast [[BYREF_A]]* [[V]] to i8*
Index: test/CodeGenObjC/blocks.m
===
--- test/CodeGenObjC/blocks.m
+++ test/CodeGenObjC/blocks.m
@@ -48,6 +48,7 @@
   // CHECK-NEXT: [[WEAKX:%.*]] = alloca [[WEAK_T:%.*]],
   // CHECK-NEXT: [[BLOCK:%.*]] = alloca [[BLOCK_T:<{.*}>]],
   // CHECK-NEXT: store [[TEST2]]*
+  // CHECK-NEXT: [[T6:%.*]] = getelementptr inbounds [[WEAK_T]], [[WEAK_T]]* [[WEAKX]], i32 0, i32 6
 
   // isa=1 for weak byrefs.
   // CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds [[WEAK_T]], [[WEAK_T]]* [[WEAKX]], i32 0, i32 0
@@ -72,7 +73,6 @@
   // CHECK-NEXT: store i8* bitcast (void (i8*)* @__Block_byref_object_dispose_{{.*}} to i8*), i8** [[T5]]
 
   // Actually capture the value.
-  // CHECK-NEXT: [[T6:%.*]] = getelementptr inbounds [[WEAK_T]], [[WEAK_T]]* [[WEAKX]], i32 0, i32 6
   // CHECK-NEXT: [[CAPTURE:%.*]] = load [[TEST2]]*, [[TEST2]]** [[X]]
   // CHECK-NEXT: store [[TEST2]]* [[CAPTURE]], [[TEST2]]** [[T6]]
 
Index: test/CodeGenObjC/blocks-2.m
===
--- test/CodeGenObjC/blocks-2.m
+++ test/CodeGenObjC/blocks-2.m
@@ -20,7 +20,7 @@
 
   // CHECK:  [[N:%.*]] = alloca [[N_T:%.*]], align 8
   // CHECK:  [[T0:%.*]] = getelementptr inbounds [[N_T]], [[N_T]]* [[N]], i32 0, i32 4
-  // CHECK-NEXT: store double 1.00e+{{0?}}01, double* [[T0]], align 8
+  // CHECK:  store double 1.00e+{{0?}}01, double* [[T0]], align 8
   __block double n = 10;
 
   // CHECK:  invoke void @{{.*}}test1_help
Index: test/CodeGenObjC/arc-blocks.m
===
--- test/CodeGenObjC/arc-blocks.m
+++ test/CodeGenObjC/arc-blocks.m
@@ -122,11 +122,11 @@
   // CHECK-LABEL:define void @test4()
   // CHECK:  [[VAR:%.*]] = alloca [[BYREF_T:%.*]],
   // CHECK-NEXT: [[BLOCK:%.*]] = alloca [[BLOCK_T:<{.*}>]],
+  // CHECK:  [[SLOT:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[VAR]], i32 0, i32 6
   // CHECK:  [[T0:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[VAR]], i32 0, i32 2
   // 0x0200 - has copy/dispose helpers strong
   // CHECK-NEXT: store i32 838860800, i32* [[T0]]
-  // CHECK:  [[SLOT:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[VAR]], i32 0, i32 6
-  // CHECK-NEXT: [[T0:%.*]] = call i8* @test4_source()
+  // CHECK:  [[T0:%.*]] = call i8* @test4_source()
   // CHECK-NEXT: [[T1:%.*]] = call i8* @llvm.objc.retainAutoreleasedReturnValue(i8* [[T0]])
   // CHECK-NEXT: store i8* [[T1]], i8** [[SLOT]]
   // CHECK-NEXT: [[SLOT:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[VAR]], i32 0, i32 6
@@ -207,11 +207,11 @@
   // CHECK-NEXT: [[BLOCK:%.*]] = alloca [[BLOCK_T:<{.*}>]],
   // CHECK-NEXT: [[VARPTR1:%.*]] = bitcast [[BYREF_T]]* [[VAR]] to i8*
   // CHECK-NEXT: call void @llvm.lifetime.start.p0i8(i64 48, i8* [[VARPTR1]])
+  // CHECK:  [[SLOT:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[VAR]], i32 0, i32 6
   // CHECK:  [[T0:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[VAR]], i32 0, i32 2
   // 0x0200 - 

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

2019-02-07 Thread 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2019-02-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:1643
 CharUnits Size = getContext().getTypeSizeInChars(type);
 if (!Size.isZero()) {
   switch (trivialAutoVarInit) {

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.


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


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

2019-02-06 Thread JF Bastien via Phabricator via cfe-commits
jfb marked 5 inline comments as done.
jfb added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:1642
 
 CharUnits Size = getContext().getTypeSizeInChars(type);
 if (!Size.isZero()) {

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];
}
```
?



Comment at: lib/CodeGen/CGDecl.cpp:1643
 CharUnits Size = getContext().getTypeSizeInChars(type);
 if (!Size.isZero()) {
   switch (trivialAutoVarInit) {

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.



Comment at: lib/CodeGen/CGDecl.cpp:1663
 const VariableArrayType *VlaType =
 dyn_cast_or_null(getContext().getAsArrayType(type));
 if (!VlaType)

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.


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


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

2019-02-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:1611
   drillIntoBlockVariable(*this, Dst, );
+}
 defaultInitNonTrivialCStructVar(Dst);

rjmccall wrote:
> Why don't you just initialize the payload right here, after we've drilled 
> down to it?
I meant to delete this comment before submitting, sorry.  I am suggesting the 
other place.


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


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

2019-02-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:1611
   drillIntoBlockVariable(*this, Dst, );
+}
 defaultInitNonTrivialCStructVar(Dst);

Why don't you just initialize the payload right here, after we've drilled down 
to it?



Comment at: lib/CodeGen/CGDecl.cpp:1642
 
 CharUnits Size = getContext().getTypeSizeInChars(type);
 if (!Size.isZero()) {

Does this check handle flexible arrays on zero-sized structures properly?  I 
suppose they're always initialized.



Comment at: lib/CodeGen/CGDecl.cpp:1643
 CharUnits Size = getContext().getTypeSizeInChars(type);
 if (!Size.isZero()) {
   switch (trivialAutoVarInit) {

Can't you just drill to the byref storage right here and avoid all the other 
complexity and subtle ordering interactions?



Comment at: lib/CodeGen/CGDecl.cpp:1663
 const VariableArrayType *VlaType =
 dyn_cast_or_null(getContext().getAsArrayType(type));
 if (!VlaType)

This is a late comment on the main patch, but there's an 
`ASTContext::getAsVariableArrayType` method.


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


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

2019-02-05 Thread JF Bastien via Phabricator via cfe-commits
jfb created this revision.
jfb added reviewers: rjmccall, pcc, kcc.
Herald added subscribers: cfe-commits, dexonsmith, jkorous.
Herald added a project: clang.

Automatic initialization [1] of __block variables was happening too late, which
caused self-init usage to crash, such as here:

typedef struct XYZ { void (^block)(); } *xyz_t;

  __attribute__((noinline))
  xyz_t create(void (^block)()) {
xyz_t myself = malloc(sizeof(struct XYZ));
myself->block = block;
return myself;
  }
  int main() {
__block xyz_t captured = create(^(){ (void)captured; });
  }

This type of code shouldn't be broken by variable auto-init, even if it's
sketchy.

This change moves some GEP generation to earlier to ensure that this
initialization occurs at the right time, hence the extra few tests being
modified.

[1] With -ftrivial-auto-var-init=pattern

rdar://problem/47798396


Repository:
  rC Clang

https://reviews.llvm.org/D57797

Files:
  lib/CodeGen/CGDecl.cpp
  test/CodeGenCXX/trivial-auto-var-init.cpp
  test/CodeGenObjC/arc-blocks.m
  test/CodeGenObjC/blocks-2.m
  test/CodeGenObjC/blocks.m
  test/CodeGenObjCXX/arc-blocks.mm

Index: test/CodeGenObjCXX/arc-blocks.mm
===
--- test/CodeGenObjCXX/arc-blocks.mm
+++ test/CodeGenObjCXX/arc-blocks.mm
@@ -24,14 +24,14 @@
   }
   // CHECK-LABEL:define void @_ZN5test03fooEv() 
   // CHECK:  [[V:%.*]] = alloca [[BYREF_A:%.*]], align 8
+  // CHECK:  [[V1:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 7
   // CHECK:  [[T0:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 4
   // CHECK-NEXT: store i8* bitcast (void (i8*, i8*)* [[COPY_HELPER:@.*]] to i8*), i8** [[T0]]
   // CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 5
   // CHECK-NEXT: store i8* bitcast (void (i8*)* [[DISPOSE_HELPER:@.*]] to i8*), i8** [[T0]]
   // CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 6
   // CHECK-NEXT: store i8* getelementptr inbounds ([3 x i8], [3 x i8]* [[LAYOUT0]], i32 0, i32 0), i8** [[T0]]
-  // CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 7
-  // CHECK-NEXT: call void @_ZN5test01AC1Ev([[A]]* [[T0]])
+  // CHECK-NEXT: call void @_ZN5test01AC1Ev([[A]]* [[V1]])
   // CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds [[BYREF_A]], [[BYREF_A]]* [[V]], i32 0, i32 7
   // CHECK: bitcast [[BYREF_A]]* [[V]] to i8*
   // CHECK: [[T1:%.*]] = bitcast [[BYREF_A]]* [[V]] to i8*
Index: test/CodeGenObjC/blocks.m
===
--- test/CodeGenObjC/blocks.m
+++ test/CodeGenObjC/blocks.m
@@ -48,6 +48,7 @@
   // CHECK-NEXT: [[WEAKX:%.*]] = alloca [[WEAK_T:%.*]],
   // CHECK-NEXT: [[BLOCK:%.*]] = alloca [[BLOCK_T:<{.*}>]],
   // CHECK-NEXT: store [[TEST2]]*
+  // CHECK-NEXT: [[T6:%.*]] = getelementptr inbounds [[WEAK_T]], [[WEAK_T]]* [[WEAKX]], i32 0, i32 6
 
   // isa=1 for weak byrefs.
   // CHECK-NEXT: [[T0:%.*]] = getelementptr inbounds [[WEAK_T]], [[WEAK_T]]* [[WEAKX]], i32 0, i32 0
@@ -72,7 +73,6 @@
   // CHECK-NEXT: store i8* bitcast (void (i8*)* @__Block_byref_object_dispose_{{.*}} to i8*), i8** [[T5]]
 
   // Actually capture the value.
-  // CHECK-NEXT: [[T6:%.*]] = getelementptr inbounds [[WEAK_T]], [[WEAK_T]]* [[WEAKX]], i32 0, i32 6
   // CHECK-NEXT: [[CAPTURE:%.*]] = load [[TEST2]]*, [[TEST2]]** [[X]]
   // CHECK-NEXT: store [[TEST2]]* [[CAPTURE]], [[TEST2]]** [[T6]]
 
Index: test/CodeGenObjC/blocks-2.m
===
--- test/CodeGenObjC/blocks-2.m
+++ test/CodeGenObjC/blocks-2.m
@@ -20,7 +20,7 @@
 
   // CHECK:  [[N:%.*]] = alloca [[N_T:%.*]], align 8
   // CHECK:  [[T0:%.*]] = getelementptr inbounds [[N_T]], [[N_T]]* [[N]], i32 0, i32 4
-  // CHECK-NEXT: store double 1.00e+{{0?}}01, double* [[T0]], align 8
+  // CHECK:  store double 1.00e+{{0?}}01, double* [[T0]], align 8
   __block double n = 10;
 
   // CHECK:  invoke void @{{.*}}test1_help
Index: test/CodeGenObjC/arc-blocks.m
===
--- test/CodeGenObjC/arc-blocks.m
+++ test/CodeGenObjC/arc-blocks.m
@@ -122,11 +122,11 @@
   // CHECK-LABEL:define void @test4()
   // CHECK:  [[VAR:%.*]] = alloca [[BYREF_T:%.*]],
   // CHECK-NEXT: [[BLOCK:%.*]] = alloca [[BLOCK_T:<{.*}>]],
+  // CHECK:  [[SLOT:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[VAR]], i32 0, i32 6
   // CHECK:  [[T0:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[VAR]], i32 0, i32 2
   // 0x0200 - has copy/dispose helpers strong
   // CHECK-NEXT: store i32 838860800, i32* [[T0]]
-  // CHECK:  [[SLOT:%.*]] = getelementptr inbounds [[BYREF_T]], [[BYREF_T]]* [[VAR]], i32 0, i32 6
-  // CHECK-NEXT: [[T0:%.*]] = call i8* @test4_source()
+  // CHECK:  [[T0:%.*]] = call i8*