[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGVTables.cpp:693
 return Int32Ty;
-  return Int8PtrTy;
+  return GlobalsInt8PtrTy;
 }

bjope wrote:
> I noticed that we have some old fixes downstream that conflicts with the 
> changes you've made here. I thought that perhaps we could get rid of those 
> now when you've fixed the code upstream.
> 
> Isn't the VTable holding function pointers when not using the relative 
> layout, and then this should be a pointer to the ProgramAddressSpace and not 
> a pointer to the DefaultGlobalsAddressSpace?
> 
> Downstream we've been using a special `FnVoidPtrTy` here. Defined as 
> `FnVoidPtrTy = Int8Ty->getPointerTo(DL.getProgramAddressSpace());`.
> 
It's a mix.  The `type_info` pointer should be in the global address space 
(although it would be forgivable to just use the default address space), the 
top, vbase, and vcall offsets are all `ptrdiff_t`s (presumably the same size as 
the default address space), and the virtual functions are function pointers.  
If we're going to support a target where those can be different sizes, we 
probably need to start computing a byte layout of the v-table and doing byte 
GEPs into it, because our current IR patterns are naively assuming the 
components are all the same size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/lib/CodeGen/CGVTables.cpp:693
 return Int32Ty;
-  return Int8PtrTy;
+  return GlobalsInt8PtrTy;
 }

I noticed that we have some old fixes downstream that conflicts with the 
changes you've made here. I thought that perhaps we could get rid of those now 
when you've fixed the code upstream.

Isn't the VTable holding function pointers when not using the relative layout, 
and then this should be a pointer to the ProgramAddressSpace and not a pointer 
to the DefaultGlobalsAddressSpace?

Downstream we've been using a special `FnVoidPtrTy` here. Defined as 
`FnVoidPtrTy = Int8Ty->getPointerTo(DL.getProgramAddressSpace());`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added inline comments.



Comment at: clang/lib/CodeGen/CGVTables.cpp:836
+fnPtr =
+llvm::ConstantExpr::getAddrSpaceCast(fnPtr, CGM.GlobalsInt8PtrTy);
+  return builder.add(fnPtr);

efriedma wrote:
> If I follow correctly, the fnPtr is guaranteed to be in the global 
> address-space, but GetAddrOfFunction returns a generic pointer.  So the 
> vtable entries are in the global address-space for efficiency? Seems 
> reasonable.
> 
> There isn't really much point to explicitly checking `FnAS != GVAS` before 
> the call to getAddrSpaceCast; getAddrSpaceCast does the same check internally.
Right, we know that these are going to be in global memory, and there is 
overhead when dealing with flat/generic. I would've liked to remove the check, 
but I think that's infeasible with how getAddrSpaceCast is currently 
implemented, because it `assert`s on `castIsValid`; addrspacecasts from the 
same AS to the same AS are invalid, so the `assert` flares on targets where 
FnAS == GVAS (e.g. x86). This is not super ergonomic IMHO, as it should be 
valid & a NOP just returning the source, but that's a change that would be 
required to allow deleting the silly check, I believe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/CodeGen/CGVTables.cpp:836
+fnPtr =
+llvm::ConstantExpr::getAddrSpaceCast(fnPtr, CGM.GlobalsInt8PtrTy);
+  return builder.add(fnPtr);

If I follow correctly, the fnPtr is guaranteed to be in the global 
address-space, but GetAddrOfFunction returns a generic pointer.  So the vtable 
entries are in the global address-space for efficiency? Seems reasonable.

There isn't really much point to explicitly checking `FnAS != GVAS` before the 
call to getAddrSpaceCast; getAddrSpaceCast does the same check internally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu 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 rG8acdcf401687: [Clang][CodeGen]`vtable`, `typeinfo` et al. 
are globals (authored by AlexVlx).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153092

Files:
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtable-align-address-space.cpp
  clang/test/CodeGenCXX/vtable-assume-load-address-space.cpp
  clang/test/CodeGenCXX/vtable-consteval-address-space.cpp
  clang/test/CodeGenCXX/vtable-constexpr-address-space.cpp
  clang/test/CodeGenCXX/vtable-key-function-address-space.cpp
  clang/test/CodeGenCXX/vtable-layout-extreme-address-space.cpp
  clang/test/CodeGenCXX/vtable-linkage-address-space.cpp
  clang/test/CodeGenCXX/vtable-pointer-initialization-address-space.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp
  clang/test/CodeGenCXX/vtt-layout-address-space.cpp
  clang/test/Headers/hip-header.hip

Index: clang/test/Headers/hip-header.hip
===
--- clang/test/Headers/hip-header.hip
+++ clang/test/Headers/hip-header.hip
@@ -43,6 +43,22 @@
 
 // expected-no-diagnostics
 
+// Check handling of overriden, implicitly __host__ dtor (should emit as a
+// nullptr to global)
+
+struct vbase {
+virtual ~vbase();
+};
+
+template
+struct vderived : public vbase {
+~vderived();
+};
+
+template struct vderived;
+
+// CHECK: @_ZTV8vderivedIvE = weak_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } zeroinitializer, comdat, align 8
+
 // Check support for pure and deleted virtual functions
 struct base {
   __host__
@@ -60,9 +76,8 @@
 __device__ void test_vf() {
 derived d;
 }
-// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @_ZN7derived2pvEv, ptr @__cxa_deleted_virtual] }, comdat, align 8
-// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @__cxa_pure_virtual, ptr @__cxa_deleted_virtual] }, comdat, align 8
-
+// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @_ZN7derived2pvEv to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
+// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @__cxa_pure_virtual to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
 // CHECK: define{{.*}}void @__cxa_pure_virtual()
 // CHECK: define{{.*}}void @__cxa_deleted_virtual()
 
Index: clang/test/CodeGenCXX/vtt-layout-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-layout-address-space.cpp
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | FileCheck %s
+
+// Test1::B should just have a single entry in its VTT, which points to the vtable.
+namespace Test1 {
+struct A { };
+
+struct B : virtual A {
+  virtual void f();
+};
+
+void B::f() { }
+}
+
+// Check that we don't add a secondary virtual pointer for Test2::A, since Test2::A doesn't have any virtual member functions or bases.
+namespace Test2 {
+  struct A { };
+
+  struct B : A { virtual void f(); };
+  struct C : virtual B { };
+
+  C c;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test3 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2, with the change suggested
+// (making A2 a virtual base of V1)
+namespace Test4 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public virtual A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { 

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 542086.
AlexVlx added a comment.

Rebase.


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

https://reviews.llvm.org/D153092

Files:
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtable-align-address-space.cpp
  clang/test/CodeGenCXX/vtable-assume-load-address-space.cpp
  clang/test/CodeGenCXX/vtable-consteval-address-space.cpp
  clang/test/CodeGenCXX/vtable-constexpr-address-space.cpp
  clang/test/CodeGenCXX/vtable-key-function-address-space.cpp
  clang/test/CodeGenCXX/vtable-layout-extreme-address-space.cpp
  clang/test/CodeGenCXX/vtable-linkage-address-space.cpp
  clang/test/CodeGenCXX/vtable-pointer-initialization-address-space.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp
  clang/test/CodeGenCXX/vtt-layout-address-space.cpp
  clang/test/Headers/hip-header.hip

Index: clang/test/Headers/hip-header.hip
===
--- clang/test/Headers/hip-header.hip
+++ clang/test/Headers/hip-header.hip
@@ -43,6 +43,22 @@
 
 // expected-no-diagnostics
 
+// Check handling of overriden, implicitly __host__ dtor (should emit as a
+// nullptr to global)
+
+struct vbase {
+virtual ~vbase();
+};
+
+template
+struct vderived : public vbase {
+~vderived();
+};
+
+template struct vderived;
+
+// CHECK: @_ZTV8vderivedIvE = weak_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } zeroinitializer, comdat, align 8
+
 // Check support for pure and deleted virtual functions
 struct base {
   __host__
@@ -60,9 +76,8 @@
 __device__ void test_vf() {
 derived d;
 }
-// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @_ZN7derived2pvEv, ptr @__cxa_deleted_virtual] }, comdat, align 8
-// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @__cxa_pure_virtual, ptr @__cxa_deleted_virtual] }, comdat, align 8
-
+// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @_ZN7derived2pvEv to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
+// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @__cxa_pure_virtual to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
 // CHECK: define{{.*}}void @__cxa_pure_virtual()
 // CHECK: define{{.*}}void @__cxa_deleted_virtual()
 
Index: clang/test/CodeGenCXX/vtt-layout-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-layout-address-space.cpp
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | FileCheck %s
+
+// Test1::B should just have a single entry in its VTT, which points to the vtable.
+namespace Test1 {
+struct A { };
+
+struct B : virtual A {
+  virtual void f();
+};
+
+void B::f() { }
+}
+
+// Check that we don't add a secondary virtual pointer for Test2::A, since Test2::A doesn't have any virtual member functions or bases.
+namespace Test2 {
+  struct A { };
+
+  struct B : A { virtual void f(); };
+  struct C : virtual B { };
+
+  C c;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test3 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2, with the change suggested
+// (making A2 a virtual base of V1)
+namespace Test4 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public virtual A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+namespace Test5 {
+  struct A {
+virtual void f() = 0;
+virtual void anchor();
+  };
+
+  void A::anchor() {
+  

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

Ping.


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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 538437.
AlexVlx added a comment.
Herald added a subscriber: wangpc.

Rebase.


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

https://reviews.llvm.org/D153092

Files:
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtable-align-address-space.cpp
  clang/test/CodeGenCXX/vtable-assume-load-address-space.cpp
  clang/test/CodeGenCXX/vtable-consteval-address-space.cpp
  clang/test/CodeGenCXX/vtable-constexpr-address-space.cpp
  clang/test/CodeGenCXX/vtable-key-function-address-space.cpp
  clang/test/CodeGenCXX/vtable-layout-extreme-address-space.cpp
  clang/test/CodeGenCXX/vtable-linkage-address-space.cpp
  clang/test/CodeGenCXX/vtable-pointer-initialization-address-space.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp
  clang/test/CodeGenCXX/vtt-layout-address-space.cpp
  clang/test/Headers/hip-header.hip

Index: clang/test/Headers/hip-header.hip
===
--- clang/test/Headers/hip-header.hip
+++ clang/test/Headers/hip-header.hip
@@ -43,6 +43,22 @@
 
 // expected-no-diagnostics
 
+// Check handling of overriden, implicitly __host__ dtor (should emit as a
+// nullptr to global)
+
+struct vbase {
+virtual ~vbase();
+};
+
+template
+struct vderived : public vbase {
+~vderived();
+};
+
+template struct vderived;
+
+// CHECK: @_ZTV8vderivedIvE = weak_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } zeroinitializer, comdat, align 8
+
 // Check support for pure and deleted virtual functions
 struct base {
   __host__
@@ -60,9 +76,8 @@
 __device__ void test_vf() {
 derived d;
 }
-// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @_ZN7derived2pvEv, ptr @__cxa_deleted_virtual] }, comdat, align 8
-// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @__cxa_pure_virtual, ptr @__cxa_deleted_virtual] }, comdat, align 8
-
+// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @_ZN7derived2pvEv to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
+// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @__cxa_pure_virtual to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
 // CHECK: define{{.*}}void @__cxa_pure_virtual()
 // CHECK: define{{.*}}void @__cxa_deleted_virtual()
 
Index: clang/test/CodeGenCXX/vtt-layout-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-layout-address-space.cpp
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | FileCheck %s
+
+// Test1::B should just have a single entry in its VTT, which points to the vtable.
+namespace Test1 {
+struct A { };
+
+struct B : virtual A {
+  virtual void f();
+};
+
+void B::f() { }
+}
+
+// Check that we don't add a secondary virtual pointer for Test2::A, since Test2::A doesn't have any virtual member functions or bases.
+namespace Test2 {
+  struct A { };
+
+  struct B : A { virtual void f(); };
+  struct C : virtual B { };
+
+  C c;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test3 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2, with the change suggested
+// (making A2 a virtual base of V1)
+namespace Test4 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public virtual A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+namespace Test5 {
+  struct A {
+virtual void f() = 0;
+virtual void 

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

Thank you @yaxunl. @rjmccall  / @efriedma any input on this? I'd like to try 
landing it next week to unblock some additional work. Thanks!


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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Yaxun Liu via Phabricator via cfe-commits]

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

In D153092#4452251 , @AlexVlx wrote:

> In D153092#4452070 , @yaxunl wrote:
>
>> This could be a good chance to switch VT to constant address space instead 
>> of global address space. AFAIK if a target has global addr space they 
>> usually also has constant addr space since they usually support OpenCL or 
>> CUDA/HIP. Is there any reason we cannot introduce a 
>> CGM.ConstantGlobalsInt8PtrTy and use it for VT instead?
>
> I did give this some thought and the benefits are somewhat unclear to the 
> point of being ultimately counterproductive. Note that these are already 
> marked `constant`, which IIRC is / was going to be enough to get most of the 
> benefits, at least on our back-end. Furthermore, the semantics of the 
> constant address space are a bit weird in something like OpenCL e.g. `A 
> pointer that points to the constant address space cannot be cast or 
> implicitly converted to the generic address space.`. This would lead to 
> weirdness when composing with CUDA / HIP, where `constant` is treated as 
> `device`, which is to say global. IIRC, you are also meant to use magical 
> interfaces to write into `constant` from the host, which a loader wouldn't 
> necessarily do. Overall, I think that the OCL formulation of `constant` is 
> actually meant to allow for relatively strange things like loading things 
> into ROM or having different pointer types (be it width or canonicity). 
> TL;DR, I am concerned that a target could validly have the `constant` addr 
> space be disjoint from generic/flat, with no viable way to even cast between 
> the two. We could say “yes, but this is not that `constant`”, but then if OCL 
> ever starts supporting dynamic polymorphism it would get confusing.

I agree that constant address space may be target dependent about what can be 
put there. I also agree there is

In D153092#4452251 , @AlexVlx wrote:

> In D153092#4452070 , @yaxunl wrote:
>
>> This could be a good chance to switch VT to constant address space instead 
>> of global address space. AFAIK if a target has global addr space they 
>> usually also has constant addr space since they usually support OpenCL or 
>> CUDA/HIP. Is there any reason we cannot introduce a 
>> CGM.ConstantGlobalsInt8PtrTy and use it for VT instead?
>
> I did give this some thought and the benefits are somewhat unclear to the 
> point of being ultimately counterproductive. Note that these are already 
> marked `constant`, which IIRC is / was going to be enough to get most of the 
> benefits, at least on our back-end. Furthermore, the semantics of the 
> constant address space are a bit weird in something like OpenCL e.g. `A 
> pointer that points to the constant address space cannot be cast or 
> implicitly converted to the generic address space.`. This would lead to 
> weirdness when composing with CUDA / HIP, where `constant` is treated as 
> `device`, which is to say global. IIRC, you are also meant to use magical 
> interfaces to write into `constant` from the host, which a loader wouldn't 
> necessarily do. Overall, I think that the OCL formulation of `constant` is 
> actually meant to allow for relatively strange things like loading things 
> into ROM or having different pointer types (be it width or canonicity). 
> TL;DR, I am concerned that a target could validly have the `constant` addr 
> space be disjoint from generic/flat, with no viable way to even cast between 
> the two. We could say “yes, but this is not that `constant`”, but then if OCL 
> ever starts supporting dynamic polymorphism it would get confusing.

Sounds reasonable.

LGTM. Thanks.


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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

In D153092#4452070 , @yaxunl wrote:

> This could be a good chance to switch VT to constant address space instead of 
> global address space. AFAIK if a target has global addr space they usually 
> also has constant addr space since they usually support OpenCL or CUDA/HIP. 
> Is there any reason we cannot introduce a CGM.ConstantGlobalsInt8PtrTy and 
> use it for VT instead?

I did give this some thought and the benefits are somewhat unclear to the point 
of being ultimately counterproductive. Note that these are already marked 
`constant`, which IIRC is / was going to be enough to get most of the benefits, 
at least on our back-end. Furthermore, the semantics of the constant address 
space are a bit weird in something like OpenCL e.g. `A pointer that points to 
the constant address space cannot be cast or implicitly converted to the 
generic address space.`. This would lead to weirdness when composing with CUDA 
/ HIP, where `constant` is treated as `device`, which is to say global. IIRC, 
you are also meant to use magical interfaces to write into `constant` from the 
host, which a loader wouldn't necessarily do. Overall, I think that the OCL 
formulation of `constant` is actually meant to allow for relatively strange 
things like loading things into ROM or having different pointer types (be it 
width or canonicity). TL;DR, I am concerned that a target could validly have 
the `constant` addr space be disjoint from generic/flat, with no viable way to 
even cast between the two. We could say “yes, but this is not that `constant`”, 
but then if OCL ever starts supporting dynamic polymorphism it would get 
confusing.


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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

This could be a good chance to switch VT to constant address space instead of 
global address space. AFAIK if a target has global addr space they usually also 
has constant addr space since they usually support OpenCL or CUDA/HIP. Is there 
any reason we cannot introduce a CGM.ConstantGlobalsInt8PtrTy and use it for VT 
instead?


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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

In D153092#4448514 , @yaxunl wrote:

> In D153092#4447445 , @AlexVlx wrote:
>
>> Fixed issue found via internal testing (thanks @yaxunl).
>
> Can we add a test to cover the regression found via internal testing? Thanks.

Done, I had forgotten about that.


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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 534670.
AlexVlx added a comment.

Add missing test for `vtable` initializers on the `__device__` side.


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

https://reviews.llvm.org/D153092

Files:
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtable-align-address-space.cpp
  clang/test/CodeGenCXX/vtable-assume-load-address-space.cpp
  clang/test/CodeGenCXX/vtable-consteval-address-space.cpp
  clang/test/CodeGenCXX/vtable-constexpr-address-space.cpp
  clang/test/CodeGenCXX/vtable-key-function-address-space.cpp
  clang/test/CodeGenCXX/vtable-layout-extreme-address-space.cpp
  clang/test/CodeGenCXX/vtable-linkage-address-space.cpp
  clang/test/CodeGenCXX/vtable-pointer-initialization-address-space.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp
  clang/test/CodeGenCXX/vtt-layout-address-space.cpp
  clang/test/Headers/hip-header.hip

Index: clang/test/Headers/hip-header.hip
===
--- clang/test/Headers/hip-header.hip
+++ clang/test/Headers/hip-header.hip
@@ -43,6 +43,22 @@
 
 // expected-no-diagnostics
 
+// Check handling of overriden, implicitly __host__ dtor (should emit as a
+// nullptr to global)
+
+struct vbase {
+virtual ~vbase();
+};
+
+template
+struct vderived : public vbase {
+~vderived();
+};
+
+template struct vderived;
+
+// CHECK: @_ZTV8vderivedIvE = weak_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } zeroinitializer, comdat, align 8
+
 // Check support for pure and deleted virtual functions
 struct base {
   __host__
@@ -60,9 +76,8 @@
 __device__ void test_vf() {
 derived d;
 }
-// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @_ZN7derived2pvEv, ptr @__cxa_deleted_virtual] }, comdat, align 8
-// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @__cxa_pure_virtual, ptr @__cxa_deleted_virtual] }, comdat, align 8
-
+// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @_ZN7derived2pvEv to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
+// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @__cxa_pure_virtual to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
 // CHECK: define{{.*}}void @__cxa_pure_virtual()
 // CHECK: define{{.*}}void @__cxa_deleted_virtual()
 
Index: clang/test/CodeGenCXX/vtt-layout-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-layout-address-space.cpp
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | FileCheck %s
+
+// Test1::B should just have a single entry in its VTT, which points to the vtable.
+namespace Test1 {
+struct A { };
+
+struct B : virtual A {
+  virtual void f();
+};
+
+void B::f() { }
+}
+
+// Check that we don't add a secondary virtual pointer for Test2::A, since Test2::A doesn't have any virtual member functions or bases.
+namespace Test2 {
+  struct A { };
+
+  struct B : A { virtual void f(); };
+  struct C : virtual B { };
+
+  C c;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test3 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2, with the change suggested
+// (making A2 a virtual base of V1)
+namespace Test4 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public virtual A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+namespace Test5 {
+  struct A {
+virtual void f() = 0;

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D153092#4447445 , @AlexVlx wrote:

> Fixed issue found via internal testing (thanks @yaxunl).

Can we add a test to cover the regression found via internal testing? Thanks.


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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 534380.
AlexVlx added a comment.

Fixed issue found via internal testing (thanks @yaxunl).


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

https://reviews.llvm.org/D153092

Files:
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtable-align-address-space.cpp
  clang/test/CodeGenCXX/vtable-assume-load-address-space.cpp
  clang/test/CodeGenCXX/vtable-consteval-address-space.cpp
  clang/test/CodeGenCXX/vtable-constexpr-address-space.cpp
  clang/test/CodeGenCXX/vtable-key-function-address-space.cpp
  clang/test/CodeGenCXX/vtable-layout-extreme-address-space.cpp
  clang/test/CodeGenCXX/vtable-linkage-address-space.cpp
  clang/test/CodeGenCXX/vtable-pointer-initialization-address-space.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp
  clang/test/CodeGenCXX/vtt-layout-address-space.cpp
  clang/test/Headers/hip-header.hip

Index: clang/test/Headers/hip-header.hip
===
--- clang/test/Headers/hip-header.hip
+++ clang/test/Headers/hip-header.hip
@@ -60,9 +60,8 @@
 __device__ void test_vf() {
 derived d;
 }
-// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @_ZN7derived2pvEv, ptr @__cxa_deleted_virtual] }, comdat, align 8
-// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @__cxa_pure_virtual, ptr @__cxa_deleted_virtual] }, comdat, align 8
-
+// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @_ZN7derived2pvEv to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
+// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @__cxa_pure_virtual to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
 // CHECK: define{{.*}}void @__cxa_pure_virtual()
 // CHECK: define{{.*}}void @__cxa_deleted_virtual()
 
Index: clang/test/CodeGenCXX/vtt-layout-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-layout-address-space.cpp
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | FileCheck %s
+
+// Test1::B should just have a single entry in its VTT, which points to the vtable.
+namespace Test1 {
+struct A { };
+
+struct B : virtual A {
+  virtual void f();
+};
+
+void B::f() { }
+}
+
+// Check that we don't add a secondary virtual pointer for Test2::A, since Test2::A doesn't have any virtual member functions or bases.
+namespace Test2 {
+  struct A { };
+
+  struct B : A { virtual void f(); };
+  struct C : virtual B { };
+
+  C c;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test3 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2, with the change suggested
+// (making A2 a virtual base of V1)
+namespace Test4 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public virtual A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+namespace Test5 {
+  struct A {
+virtual void f() = 0;
+virtual void anchor();
+  };
+
+  void A::anchor() {
+  }
+}
+
+namespace Test6 {
+  struct A {
+virtual void f() = delete;
+virtual void anchor();
+  };
+
+  void A::anchor() {
+  }
+}
+
+// CHECK: @_ZTTN5Test11BE ={{.*}} unnamed_addr addrspace(1) constant [1 x ptr addrspace(1)] [ptr addrspace(1) getelementptr inbounds ({ [4 x ptr addrspace(1)] }, ptr addrspace(1) @_ZTVN5Test11BE, i32 0, inrange i32 0, i32 3)]
+// CHECK: @_ZTVN5Test51AE ={{.*}} unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { 

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

Gentle ping.


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

https://reviews.llvm.org/D153092

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 532470.
AlexVlx added a comment.

`clang-format`


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

https://reviews.llvm.org/D153092

Files:
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtable-align-address-space.cpp
  clang/test/CodeGenCXX/vtable-assume-load-address-space.cpp
  clang/test/CodeGenCXX/vtable-consteval-address-space.cpp
  clang/test/CodeGenCXX/vtable-constexpr-address-space.cpp
  clang/test/CodeGenCXX/vtable-key-function-address-space.cpp
  clang/test/CodeGenCXX/vtable-layout-extreme-address-space.cpp
  clang/test/CodeGenCXX/vtable-linkage-address-space.cpp
  clang/test/CodeGenCXX/vtable-pointer-initialization-address-space.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp
  clang/test/CodeGenCXX/vtt-layout-address-space.cpp
  clang/test/Headers/hip-header.hip

Index: clang/test/Headers/hip-header.hip
===
--- clang/test/Headers/hip-header.hip
+++ clang/test/Headers/hip-header.hip
@@ -60,9 +60,8 @@
 __device__ void test_vf() {
 derived d;
 }
-// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @_ZN7derived2pvEv, ptr @__cxa_deleted_virtual] }, comdat, align 8
-// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @__cxa_pure_virtual, ptr @__cxa_deleted_virtual] }, comdat, align 8
-
+// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @_ZN7derived2pvEv to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
+// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @__cxa_pure_virtual to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
 // CHECK: define{{.*}}void @__cxa_pure_virtual()
 // CHECK: define{{.*}}void @__cxa_deleted_virtual()
 
Index: clang/test/CodeGenCXX/vtt-layout-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-layout-address-space.cpp
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | FileCheck %s
+
+// Test1::B should just have a single entry in its VTT, which points to the vtable.
+namespace Test1 {
+struct A { };
+
+struct B : virtual A {
+  virtual void f();
+};
+
+void B::f() { }
+}
+
+// Check that we don't add a secondary virtual pointer for Test2::A, since Test2::A doesn't have any virtual member functions or bases.
+namespace Test2 {
+  struct A { };
+
+  struct B : A { virtual void f(); };
+  struct C : virtual B { };
+
+  C c;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test3 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2, with the change suggested
+// (making A2 a virtual base of V1)
+namespace Test4 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public virtual A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+namespace Test5 {
+  struct A {
+virtual void f() = 0;
+virtual void anchor();
+  };
+
+  void A::anchor() {
+  }
+}
+
+namespace Test6 {
+  struct A {
+virtual void f() = delete;
+virtual void anchor();
+  };
+
+  void A::anchor() {
+  }
+}
+
+// CHECK: @_ZTTN5Test11BE ={{.*}} unnamed_addr addrspace(1) constant [1 x ptr addrspace(1)] [ptr addrspace(1) getelementptr inbounds ({ [4 x ptr addrspace(1)] }, ptr addrspace(1) @_ZTVN5Test11BE, i32 0, inrange i32 0, i32 3)]
+// CHECK: @_ZTVN5Test51AE ={{.*}} unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) 

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 532294.
AlexVlx added a comment.

Rebased.


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

https://reviews.llvm.org/D153092

Files:
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtable-align-address-space.cpp
  clang/test/CodeGenCXX/vtable-assume-load-address-space.cpp
  clang/test/CodeGenCXX/vtable-consteval-address-space.cpp
  clang/test/CodeGenCXX/vtable-constexpr-address-space.cpp
  clang/test/CodeGenCXX/vtable-key-function-address-space.cpp
  clang/test/CodeGenCXX/vtable-layout-extreme-address-space.cpp
  clang/test/CodeGenCXX/vtable-linkage-address-space.cpp
  clang/test/CodeGenCXX/vtable-pointer-initialization-address-space.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp
  clang/test/CodeGenCXX/vtt-layout-address-space.cpp
  clang/test/Headers/hip-header.hip

Index: clang/test/Headers/hip-header.hip
===
--- clang/test/Headers/hip-header.hip
+++ clang/test/Headers/hip-header.hip
@@ -60,9 +60,8 @@
 __device__ void test_vf() {
 derived d;
 }
-// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @_ZN7derived2pvEv, ptr @__cxa_deleted_virtual] }, comdat, align 8
-// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @__cxa_pure_virtual, ptr @__cxa_deleted_virtual] }, comdat, align 8
-
+// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @_ZN7derived2pvEv to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
+// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @__cxa_pure_virtual to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
 // CHECK: define{{.*}}void @__cxa_pure_virtual()
 // CHECK: define{{.*}}void @__cxa_deleted_virtual()
 
Index: clang/test/CodeGenCXX/vtt-layout-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-layout-address-space.cpp
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | FileCheck %s
+
+// Test1::B should just have a single entry in its VTT, which points to the vtable.
+namespace Test1 {
+struct A { };
+
+struct B : virtual A {
+  virtual void f();
+};
+
+void B::f() { }
+}
+
+// Check that we don't add a secondary virtual pointer for Test2::A, since Test2::A doesn't have any virtual member functions or bases.
+namespace Test2 {
+  struct A { };
+
+  struct B : A { virtual void f(); };
+  struct C : virtual B { };
+
+  C c;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test3 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2, with the change suggested
+// (making A2 a virtual base of V1)
+namespace Test4 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public virtual A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+namespace Test5 {
+  struct A {
+virtual void f() = 0;
+virtual void anchor();
+  };
+
+  void A::anchor() {
+  }
+}
+
+namespace Test6 {
+  struct A {
+virtual void f() = delete;
+virtual void anchor();
+  };
+
+  void A::anchor() {
+  }
+}
+
+// CHECK: @_ZTTN5Test11BE ={{.*}} unnamed_addr addrspace(1) constant [1 x ptr addrspace(1)] [ptr addrspace(1) getelementptr inbounds ({ [4 x ptr addrspace(1)] }, ptr addrspace(1) @_ZTVN5Test11BE, i32 0, inrange i32 0, i32 3)]
+// CHECK: @_ZTVN5Test51AE ={{.*}} unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, 

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx created this revision.
AlexVlx added reviewers: rjmccall, efriedma, yaxunl.
Herald added subscribers: arichardson, tpr.
Herald added a project: All.
AlexVlx requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

All data structures and values associated with handling virtual functions / 
inheritance, as well as RTTI, are globals and thus can only reside in the 
`global` address space. This was not taken fully taken into account because for 
most targets, `global` & `generic` appear to coincide. However, on targets 
where `global` & `generic` `AS`es differ (e.g. AMDGPU), this was problematic, 
since it led to the generation of invalid `bitcast`s (which would trigger 
`assert`s in Debug) and less than optimal code. This patch does two things:

- ensures that `vtable`s, `vptr`s, `vtt`s, `typeinfo` are generated in the 
right AS, and populated accordingly;
- removes a bunch of `bitcast`s which look like left-overs from the typed ptr 
era.

This is a bit more noisy than I'd have liked, but functionality is somewhat 
spread out. There's one bit of less than ideal code, stemming from the fact 
that functions are in the `generic` AS, and thus it's necessary to insert a 
`constexpr` cast from `generic` to `global` when populating the `vtable`. 
Adjusting appears disruptive enough to prefer to do it separately (unless I 
missed something obvious).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153092

Files:
  clang/lib/CodeGen/CGVTT.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtable-align-address-space.cpp
  clang/test/CodeGenCXX/vtable-assume-load-address-space.cpp
  clang/test/CodeGenCXX/vtable-consteval-address-space.cpp
  clang/test/CodeGenCXX/vtable-constexpr-address-space.cpp
  clang/test/CodeGenCXX/vtable-key-function-address-space.cpp
  clang/test/CodeGenCXX/vtable-layout-extreme-address-space.cpp
  clang/test/CodeGenCXX/vtable-linkage-address-space.cpp
  clang/test/CodeGenCXX/vtable-pointer-initialization-address-space.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp
  clang/test/CodeGenCXX/vtt-layout-address-space.cpp
  clang/test/Headers/hip-header.hip

Index: clang/test/Headers/hip-header.hip
===
--- clang/test/Headers/hip-header.hip
+++ clang/test/Headers/hip-header.hip
@@ -60,9 +60,8 @@
 __device__ void test_vf() {
 derived d;
 }
-// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @_ZN7derived2pvEv, ptr @__cxa_deleted_virtual] }, comdat, align 8
-// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @__cxa_pure_virtual, ptr @__cxa_deleted_virtual] }, comdat, align 8
-
+// CHECK: @_ZTV7derived = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @_ZN7derived2pvEv to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
+// CHECK: @_ZTV4base = linkonce_odr unnamed_addr addrspace(1) constant { [4 x ptr addrspace(1)] } { [4 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) null, ptr addrspace(1) addrspacecast (ptr @__cxa_pure_virtual to ptr addrspace(1)), ptr addrspace(1) addrspacecast (ptr @__cxa_deleted_virtual to ptr addrspace(1))] }, comdat, align 8
 // CHECK: define{{.*}}void @__cxa_pure_virtual()
 // CHECK: define{{.*}}void @__cxa_deleted_virtual()
 
Index: clang/test/CodeGenCXX/vtt-layout-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-layout-address-space.cpp
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | FileCheck %s
+
+// Test1::B should just have a single entry in its VTT, which points to the vtable.
+namespace Test1 {
+struct A { };
+
+struct B : virtual A {
+  virtual void f();
+};
+
+void B::f() { }
+}
+
+// Check that we don't add a secondary virtual pointer for Test2::A, since Test2::A doesn't have any virtual member functions or bases.
+namespace Test2 {
+  struct A { };
+
+  struct B : A { virtual void f(); };
+  struct C : virtual B { };
+
+  C c;
+}
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test3 {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2,