[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd60c3d08e78d: [clang] Skip stores in init for fields that 
are empty structs (authored by mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D157332?vs=549575=550209#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenCXX/ctor-empty-nounique.cpp


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | 
FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated 
for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns 
as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+
+// Check that the constructor for an empty member gets called with the right
+// 'this' pointer.
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
@@ -5781,9 +5782,14 @@
 DestIsVolatile = false;
   }
 
-  // If the value is offset in memory, apply the offset now.
-  Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
-  CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  // An empty record can overlap other data (if declared with
+  // no_unique_address); omit the store for such types - as there is no
+  // actual data to store.
+  if (!isEmptyRecord(getContext(), RetTy, true)) {
+// If the value is offset in memory, apply the offset now.
+Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
+CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  }
 
   return convertTempToRValue(DestPtr, RetTy, SourceLocation());
 }


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+
+// Check that the constructor for an empty member gets called with the right
+// 'this' pointer.
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo marked an inline comment as done.
mstorsjo added a comment.

Thanks a lot for the guidance!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 549575.
mstorsjo added a comment.

Move the existing comment into the new if statement, add a comment to the new 
if. Add a comment to the second part of the testcase, which probably is good to 
keep anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenCXX/ctor-empty-nounique.cpp


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | 
FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated 
for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns 
as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+
+// Check that the constructor for an empty member gets called with the right
+// 'this' pointer.
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
@@ -5777,9 +5778,13 @@
 DestIsVolatile = false;
   }
 
-  // If the value is offset in memory, apply the offset now.
-  Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
-  CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  // An empty record can overlap other data (if declared with 
no_unique_address);
+  // omit the store for such types - as there is no actual data to store.
+  if (!isEmptyRecord(getContext(), RetTy, true)) {
+// If the value is offset in memory, apply the offset now.
+Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
+CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  }
 
   return convertTempToRValue(DestPtr, RetTy, SourceLocation());
 }


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+
+// Check that the constructor for an empty member gets called with the right
+// 'this' pointer.
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include 

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

This approach looks fine.




Comment at: clang/lib/CodeGen/CGCall.cpp:5781
 
   // If the value is offset in memory, apply the offset now.
+  if (!isEmptyRecord(getContext(), RetTy, true)) {

The isEmptyRecord call could use a comment briefly explaining that empty 
records can overlap with other data.

The existing comment about the offset probably belongs inside the if statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 549517.
mstorsjo added a comment.

Updated to just check isEmptyRecord in EmitCall.

The second part of the test probably is kinda redundant/pointless at this 
point, and I guess the test comment needs updating too; can do that later when 
the implementation is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenCXX/ctor-empty-nounique.cpp


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | 
FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated 
for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns 
as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
@@ -5778,8 +5779,10 @@
   }
 
   // If the value is offset in memory, apply the offset now.
-  Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
-  CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  if (!isEmptyRecord(getContext(), RetTy, true)) {
+Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
+CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  }
 
   return convertTempToRValue(DestPtr, RetTy, SourceLocation());
 }


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -13,6 +13,7 @@
 
 #include "CGCall.h"
 #include "ABIInfo.h"
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGCleanup.h"
@@ -5778,8 +5779,10 @@
   }
 
   // If the value is offset in memory, apply the offset now.
-  Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);
-  CreateCoercedStore(CI, StorePtr, DestIsVolatile, *this);
+  if (!isEmptyRecord(getContext(), RetTy, true)) {
+Address StorePtr = emitAddressAtOffset(*this, DestPtr, RetAI);

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

You can't, in general, check whether a type is stored in a no_unique_address 
field.  Consider the following; the bad store is in the constructor S2, but the 
relevant no_unique_address attribute doesn't even show up until the definition 
of S3.

  struct S {};
  S f();
  struct S2 : public S { S2();};
  S2::S2() : S(f()) {}
  struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
  S3::S3() : x(1), y() {}

So we have to suppress all stores of empty types, whether or not we see a 
no_unique_address attribute.

Given that, I don't think the isDummy field is necessary; the important part is 
just whether the type is empty, and you can derive that directly from the type 
itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 549099.
mstorsjo added a comment.

Plumb the info from EmitInitializerForField through AggValueSlot and 
ReturnValueSlot to EmitCall.

The fields/variables could use better names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGCall.h
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGValue.h
  clang/test/CodeGenCXX/ctor-empty-nounique.cpp

Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64le-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: ret void
+
+struct S2 {
+  S2();
+};
+struct Z2 {
+  int x;
+  [[no_unique_address]] S2 y;
+  Z2();
+};
+Z2::Z2() : x(111) {}
+
+// CHECK: define {{.*}} @_ZN2Z2C2Ev(ptr {{.*}} %this)
+// CHECK: %this.addr = alloca ptr
+// CHECK: store ptr %this, ptr %this.addr
+// CHECK: %this1 = load ptr, ptr %this.addr
+// CHECK: call void @_ZN2S2C1Ev(ptr {{.*}} %this1)
Index: clang/lib/CodeGen/CGValue.h
===
--- clang/lib/CodeGen/CGValue.h
+++ clang/lib/CodeGen/CGValue.h
@@ -559,13 +559,16 @@
   /// them.
   bool SanitizerCheckedFlag : 1;
 
+  bool IsDummy : 1;
+
+public:
   AggValueSlot(Address Addr, Qualifiers Quals, bool DestructedFlag,
bool ObjCGCFlag, bool ZeroedFlag, bool AliasedFlag,
-   bool OverlapFlag, bool SanitizerCheckedFlag)
+   bool OverlapFlag, bool SanitizerCheckedFlag, bool IsDummy)
   : Addr(Addr), Quals(Quals), DestructedFlag(DestructedFlag),
 ObjCGCFlag(ObjCGCFlag), ZeroedFlag(ZeroedFlag),
 AliasedFlag(AliasedFlag), OverlapFlag(OverlapFlag),
-SanitizerCheckedFlag(SanitizerCheckedFlag) {}
+SanitizerCheckedFlag(SanitizerCheckedFlag), IsDummy(IsDummy) {}
 
 public:
   enum IsAliased_t { IsNotAliased, IsAliased };
@@ -574,6 +577,7 @@
   enum Overlap_t { DoesNotOverlap, MayOverlap };
   enum NeedsGCBarriers_t { DoesNotNeedGCBarriers, NeedsGCBarriers };
   enum IsSanitizerChecked_t { IsNotSanitizerChecked, IsSanitizerChecked };
+  enum IsDummy_t { IsNotADummySlot, IsADummySlot };
 
   /// ignored - Returns an aggregate value slot indicating that the
   /// aggregate value is being ignored.
@@ -592,27 +596,26 @@
   ///   for calling destructors on this object
   /// \param needsGC - true if the slot is potentially located
   ///   somewhere that ObjC GC calls should be emitted for
-  static AggValueSlot forAddr(Address addr,
-  Qualifiers quals,
-  IsDestructed_t isDestructed,
-  NeedsGCBarriers_t needsGC,
-  IsAliased_t isAliased,
-  Overlap_t mayOverlap,
-  IsZeroed_t isZeroed = IsNotZeroed,
-   IsSanitizerChecked_t isChecked = IsNotSanitizerChecked) {
+  static AggValueSlot
+  forAddr(Address addr, Qualifiers quals, IsDestructed_t isDestructed,
+  NeedsGCBarriers_t needsGC, IsAliased_t isAliased,
+  Overlap_t mayOverlap, IsZeroed_t isZeroed = IsNotZeroed,
+  IsSanitizerChecked_t isChecked = IsNotSanitizerChecked,
+  IsDummy_t isDummy = IsNotADummySlot) {
 if (addr.isValid())
   addr.setKnownNonNull();
 return AggValueSlot(addr, quals, isDestructed, needsGC, isZeroed, isAliased,
-mayOverlap, isChecked);
+mayOverlap, isChecked, isDummy);
   }
 
   static AggValueSlot
   forLValue(const LValue , CodeGenFunction , IsDestructed_t isDestructed,
 NeedsGCBarriers_t needsGC, IsAliased_t isAliased,
 Overlap_t mayOverlap, IsZeroed_t isZeroed = IsNotZeroed,
-IsSanitizerChecked_t isChecked = IsNotSanitizerChecked) {
+IsSanitizerChecked_t isChecked = IsNotSanitizerChecked,
+IsDummy_t isDummy = 

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D157332#4570290 , @crtrott wrote:

> Question: does this bug potentially affect code generation for 
> AMD/NVIDIA/Intel GPUs?

I believe the easiest way to test that is to try compiling `struct S {}; S 
ret() { return S(); }` into IR and checking the signature - if it returns void, 
the target should be immune to this bug, otherwise the bug probably is present.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Christian Trott via Phabricator via cfe-commits]

crtrott added a comment.

To answer my own question: I was able to reproduce the bug when compiling for 
NVIDIA GPUs, I was not able to reproduce it for AMD GPUs yet, and I didn't try 
for Intel GPUs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Christian Trott via Phabricator via cfe-commits]

crtrott added a comment.

Question: does this bug potentially affect code generation for AMD/NVIDIA/Intel 
GPUs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D157332#4568341 , @efriedma wrote:

> I see what you're getting at here... but I don't think this works quite 
> right.  If the empty class has a non-trivial constructor, we have to pass the 
> correct "this" address to that constructor.  Usually a constructor for an 
> empty class won't do anything with that address, but it could theoretically 
> do something with it.

Hmm, I think I see. In this specific case, there's no constructor for the empty 
class invoked at all, we call the function `f()` which returns an `struct S` 
(which is empty). But if we'd remove the initialization of `y(f())` and add a 
constructor to `S()`, I see that we generate code that is indeed wrong in that 
aspect.

> In order to preserve the address in the cases we need it, we need a different 
> invariant: the handling for each aggregate expression in EmitAggExpr needs to 
> ensure it doesn't store anything to an empty class.  Which is unfortunately 
> more fragile than I'd like, but I can't think of a better approach.  This 
> check needs to happen further down the call stack.  Maybe put it in 
> CodeGenFunction::EmitCall.

Ok, I'll see if I can look further into that... (I don't have a huge amount of 
time for it at the moment, so if someone else with an interest in the area, 
e.g. @crtrott or @amyk want to dig into it, feel free!)




Comment at: clang/test/CodeGenCXX/ctor-empty-nounique.cpp:1
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+

cor3ntin wrote:
> This should probably get another run line for powerpc64le
Sure, I could do that. (Initially I had both, but thought people would consider 
it unnecessary duplication.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/test/CodeGenCXX/ctor-empty-nounique.cpp:1
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+

This should probably get another run line for powerpc64le


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I see what you're getting at here... but I don't think this works quite right.  
If the empty class has a non-trivial constructor, we have to pass the correct 
"this" address to that constructor.  Usually a constructor for an empty class 
won't do anything with that address, but it could theoretically do something 
with it.

In order to preserve the address in the cases we need it, we need a different 
invariant: the handling for each aggregate expression in EmitAggExpr needs to 
ensure it doesn't store anything to an empty class.  Which is unfortunately 
more fragile than I'd like, but I can't think of a better approach.  This check 
needs to happen further down the call stack.  Maybe put it in 
CodeGenFunction::EmitCall.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157332

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

[PATCH] D157332: [clang] Make init for empty no_unique_address fields a no-op write

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: aaron.ballman, efriedma.
Herald added subscribers: steven.zhang, pengfei.
Herald added a project: All.
mstorsjo requested review of this revision.
Herald added a project: clang.

An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

The existing clang tests in CodeGenCXX/tail-padding.cpp contain
cases of fields that have no_unique_address on non-empty struct
members; the check for isEmptyRecord() is needed to retain the
previous behaviour in that test.

Fixes https://github.com/llvm/llvm-project/issues/64253, and
possibly https://github.com/llvm/llvm-project/issues/64077
and https://github.com/llvm/llvm-project/issues/64427 as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157332

Files:
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGenCXX/ctor-empty-nounique.cpp


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated 
for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns 
as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+
+// CHECK: %coerce = alloca %struct.S, align 1
+
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: %coerce.dive = getelementptr inbounds %struct.S, ptr %coerce, 
i32 0, i32 0
+// CHECK-NEXT: store i8 %call, ptr %coerce.dive, align 1
+// CHECK-NEXT: ret void
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -10,6 +10,7 @@
 //
 
//===--===//
 
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGDebugInfo.h"
@@ -701,6 +702,9 @@
 getOverlapForFieldInit(Field), AggValueSlot::IsNotZeroed,
 // Checks are made by the code that calls constructor.
 AggValueSlot::IsSanitizerChecked);
+if (Field->hasAttr() &&
+isEmptyRecord(getContext(), FieldType, true))
+  Slot = AggValueSlot::ignored();
 EmitAggExpr(Init, Slot);
 break;
   }


Index: clang/test/CodeGenCXX/ctor-empty-nounique.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/ctor-empty-nounique.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s | FileCheck %s
+
+// An empty struct is handled as a struct with a dummy i8, on all targets.
+// Most targets treat an empty struct return value as essentially void - but
+// some don't. (Currently, at least x86_64-windows-* and powerpc64le-* don't
+// treat it as void.)
+//
+// When intializing a struct with such a no_unique_address member, make sure we
+// don't write the dummy i8 into the struct where there's no space allocated for
+// it.
+//
+// This can only be tested with targets that don't treat empty struct returns as
+// void.
+
+struct S {};
+S f();
+struct Z {
+  int x;
+  [[no_unique_address]] S y;
+  Z();
+};
+Z::Z() : x(111), y(f()) {}
+
+// CHECK: define {{.*}} @_ZN1ZC2Ev
+
+// CHECK: %coerce = alloca %struct.S, align 1
+
+// CHECK: %call = call i8 @_Z1fv()
+// CHECK-NEXT: %coerce.dive = getelementptr inbounds %struct.S, ptr %coerce, i32 0, i32 0
+// CHECK-NEXT: store i8 %call, ptr %coerce.dive, align 1
+// CHECK-NEXT: ret void
Index: clang/lib/CodeGen/CGClass.cpp
===
--- clang/lib/CodeGen/CGClass.cpp
+++ clang/lib/CodeGen/CGClass.cpp
@@ -10,6 +10,7 @@
 //
 //===--===//
 
+#include "ABIInfoImpl.h"
 #include "CGBlocks.h"
 #include "CGCXXABI.h"
 #include "CGDebugInfo.h"
@@ -701,6 +702,9 @@
 getOverlapForFieldInit(Field), AggValueSlot::IsNotZeroed,
 // Checks are made by the code that calls