[Clang][CodeGen] Start migrating away from assuming the Default AS is 0 #88182
Conversation
…all_defaults_are_zero
…x/llvm-project; branch 'main' of https://github.com/llvm/llvm-project into not_all_defaults_are_zero
…all_defaults_are_zero
…all_defaults_are_zero
…all_defaults_are_zero
…all_defaults_are_zero
…all_defaults_are_zero
…all_defaults_are_zero
…all_defaults_are_zero
…all_defaults_are_zero
…all_defaults_are_zero
… add (more) tests.
AlexVlx
requested review from
efriedma-quic,
yxsamliu,
arichardson,
rjmccall,
bjope and
nunoplopes
|
@llvm/pr-subscribers-clang-codegen Author: Alex Voicu (AlexVlx) ChangesAt the moment, Clang is rather liberal in assuming that 0 (and by extension unqualified) is always a safe default. This does not work for targets that actually use a different value for the default / generic AS (for example, the SPIRV that obtains from HIPSPV or SYCL). This patch is a first step, fairly safe step towards trying to clear things up by:
A bunch of tests (adapted from existing ones) are added. I've opted against adding new cases within to the existing ones sinc e some are fairly verbose already. Patch is 67.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88182.diff 21 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index a4fb673284ceca..6c8d7804216b52 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -2216,7 +2216,7 @@ static llvm::Value *EmitTypeidFromVTable(CodeGenFunction &CGF, const Expr *E,
}
llvm::Value *CodeGenFunction::EmitCXXTypeidExpr(const CXXTypeidExpr *E) {
- llvm::Type *PtrTy = llvm::PointerType::getUnqual(getLLVMContext());
+ llvm::Type *PtrTy = Int8PtrTy;
LangAS GlobAS = CGM.GetGlobalVarAddressSpace(nullptr);
auto MaybeASCast = [=](auto &&TypeInfo) {
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 00b3bfcaa0bc25..158dca114679ce 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -366,12 +366,13 @@ CodeGenModule::CodeGenModule(ASTContext &C,
IntTy = llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getIntWidth());
IntPtrTy = llvm::IntegerType::get(LLVMContext,
C.getTargetInfo().getMaxPointerWidth());
- Int8PtrTy = llvm::PointerType::get(LLVMContext, 0);
+ Int8PtrTy = llvm::PointerType::get(LLVMContext,
+ C.getTargetAddressSpace(LangAS::Default));
const llvm::DataLayout &DL = M.getDataLayout();
AllocaInt8PtrTy =
llvm::PointerType::get(LLVMContext, DL.getAllocaAddrSpace());
- GlobalsInt8PtrTy =
- llvm::PointerType::get(LLVMContext, DL.getDefaultGlobalsAddressSpace());
+ GlobalsInt8PtrTy = llvm::PointerType::get(
+ LLVMContext, C.getTargetAddressSpace(GetGlobalVarAddressSpace(nullptr)));
ConstGlobalsPtrTy = llvm::PointerType::get(
LLVMContext, C.getTargetAddressSpace(GetGlobalConstantAddressSpace()));
ASTAllocaAddressSpace = getTargetCodeGenInfo().getASTAllocaAddressSpace();
@@ -3582,7 +3583,9 @@ ConstantAddress CodeGenModule::GetAddrOfTemplateParamObject(
? llvm::GlobalValue::LinkOnceODRLinkage
: llvm::GlobalValue::InternalLinkage;
auto *GV = new llvm::GlobalVariable(getModule(), Init->getType(),
- /*isConstant=*/true, Linkage, Init, Name);
+ /*isConstant=*/true, Linkage, Init, Name,
+ nullptr, llvm::GlobalValue::NotThreadLocal,
+ GlobalsInt8PtrTy->getAddressSpace());
setGVProperties(GV, TPO);
if (supportsCOMDAT())
GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
@@ -4549,9 +4552,10 @@ llvm::Constant *CodeGenModule::GetOrCreateLLVMFunction(
IsIncompleteFunction = true;
}
- llvm::Function *F =
- llvm::Function::Create(FTy, llvm::Function::ExternalLinkage,
- Entry ? StringRef() : MangledName, &getModule());
+ llvm::Function *F = llvm::Function::Create(
+ FTy, llvm::Function::ExternalLinkage,
+ getDataLayout().getProgramAddressSpace(),
+ Entry ? StringRef() : MangledName, &getModule());
// Store the declaration associated with this function so it is potentially
// updated by further declarations or definitions and emitted at the end.
@@ -5022,7 +5026,9 @@ llvm::GlobalVariable *CodeGenModule::CreateOrReplaceCXXRuntimeVariable(
// Create a new variable.
GV = new llvm::GlobalVariable(getModule(), Ty, /*isConstant=*/true,
- Linkage, nullptr, Name);
+ Linkage, nullptr, Name, nullptr,
+ llvm::GlobalValue::NotThreadLocal,
+ GlobalsInt8PtrTy->getAddressSpace());
if (OldGV) {
// Replace occurrences of the old variable if needed.
@@ -5137,7 +5143,7 @@ LangAS CodeGenModule::GetGlobalVarAddressSpace(const VarDecl *D) {
return LangAS::cuda_device;
}
- if (LangOpts.OpenMP) {
+ if (LangOpts.OpenMP && OpenMPRuntime) {
LangAS AS;
if (OpenMPRuntime->hasAllocateAttributeForGlobalVar(D, AS))
return AS;
diff --git a/clang/lib/CodeGen/CodeGenTypeCache.h b/clang/lib/CodeGen/CodeGenTypeCache.h
index 083d69214fb3c2..e273ebe3b060f2 100644
--- a/clang/lib/CodeGen/CodeGenTypeCache.h
+++ b/clang/lib/CodeGen/CodeGenTypeCache.h
@@ -51,7 +51,7 @@ struct CodeGenTypeCache {
llvm::IntegerType *PtrDiffTy;
};
- /// void*, void** in address space 0
+ /// void*, void** in the target's default address space (often 0)
union {
llvm::PointerType *UnqualPtrTy;
llvm::PointerType *VoidPtrTy;
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 18acf7784f714b..942ab4c3a43d05 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -3279,7 +3279,9 @@ ItaniumRTTIBuilder::GetAddrOfExternalRTTIDescriptor(QualType Ty) {
GV = new llvm::GlobalVariable(
CGM.getModule(), CGM.GlobalsInt8PtrTy,
- /*isConstant=*/true, llvm::GlobalValue::ExternalLinkage, nullptr, Name);
+ /*isConstant=*/true, llvm::GlobalValue::ExternalLinkage, nullptr, Name,
+ nullptr, llvm::GlobalValue::NotThreadLocal,
+ CGM.GlobalsInt8PtrTy->getAddressSpace());
const CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
CGM.setGVProperties(GV, RD);
// Import the typeinfo symbol when all non-inline virtual methods are
@@ -3673,8 +3675,18 @@ void ItaniumRTTIBuilder::BuildVTablePointer(const Type *Ty) {
if (CGM.getItaniumVTableContext().isRelativeLayout())
VTable = CGM.getModule().getNamedAlias(VTableName);
if (!VTable) {
- llvm::Type *Ty = llvm::ArrayType::get(CGM.GlobalsInt8PtrTy, 0);
- VTable = CGM.getModule().getOrInsertGlobal(VTableName, Ty);
+ VTable = CGM.GetGlobalValue(VTableName);
+ if (!VTable) {
+ llvm::Type *Ty = llvm::ArrayType::get(CGM.GlobalsInt8PtrTy, 0);
+ // FIXME: External StdLib VTables should be constant as well, but changing
+ // it *might* constitute a very subtle ABI break.
+ VTable = new llvm::GlobalVariable(CGM.getModule(), Ty,
+ /*isConstant=*/false,
+ llvm::GlobalVariable::ExternalLinkage,
+ nullptr, VTableName, nullptr,
+ llvm::GlobalValue::NotThreadLocal,
+ CGM.GlobalsInt8PtrTy->getAddressSpace());
+ }
}
CGM.setDSOLocal(cast<llvm::GlobalValue>(VTable->stripPointerCasts()));
@@ -3934,7 +3946,9 @@ llvm::Constant *ItaniumRTTIBuilder::BuildTypeInfo(
llvm::GlobalVariable *OldGV = M.getNamedGlobal(Name);
llvm::GlobalVariable *GV =
new llvm::GlobalVariable(M, Init->getType(),
- /*isConstant=*/true, Linkage, Init, Name);
+ /*isConstant=*/true, Linkage, Init, Name,
+ nullptr, llvm::GlobalValue::NotThreadLocal,
+ CGM.GlobalsInt8PtrTy->getAddressSpace());
// Export the typeinfo in the same circumstances as the vtable is exported.
auto GVDLLStorageClass = DLLStorageClass;
diff --git a/clang/test/CodeGenCXX/dynamic-cast-nonzero-default-address-space.cpp b/clang/test/CodeGenCXX/dynamic-cast-nonzero-default-address-space.cpp
new file mode 100644
index 00000000000000..9a9fbdbd14582d
--- /dev/null
+++ b/clang/test/CodeGenCXX/dynamic-cast-nonzero-default-address-space.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -I%S %s -triple spirv64-unknown-unknown -fsycl-is-device -emit-llvm -fcxx-exceptions -fexceptions -o - | FileCheck %s
+struct A { virtual void f(); };
+struct B : A { };
+
+// CHECK: {{define.*@_Z1fP1A}}
+// CHECK-SAME: personality ptr @__gxx_personality_v0
+B fail;
+const B& f(A *a) {
+ try {
+ // CHECK: call {{.*}} ptr addrspace(4) @__dynamic_cast
+ // CHECK: br i1
+ // CHECK: invoke {{.*}} void @__cxa_bad_cast() [[NR:#[0-9]+]]
+ dynamic_cast<const B&>(*a);
+ } catch (...) {
+ // CHECK: landingpad { ptr addrspace(4), i32 }
+ // CHECK-NEXT: catch ptr addrspace(4) null
+ }
+ return fail;
+}
+
+// CHECK: declare {{.*}} ptr addrspace(4) @__dynamic_cast(ptr addrspace(4), ptr addrspace(1), ptr addrspace(1), i64) [[NUW_RO:#[0-9]+]]
+
+// CHECK: attributes [[NUW_RO]] = { nounwind willreturn memory(read) }
+// CHECK: attributes [[NR]] = { noreturn }
diff --git a/clang/test/CodeGenCXX/template-param-objects-nonzero-default-address-space.cpp b/clang/test/CodeGenCXX/template-param-objects-nonzero-default-address-space.cpp
new file mode 100644
index 00000000000000..9fc95acaebc7bf
--- /dev/null
+++ b/clang/test/CodeGenCXX/template-param-objects-nonzero-default-address-space.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -triple spirv64-unknown-unknown -fsycl-is-device -std=c++20 %s -emit-llvm -o - | FileCheck %s
+
+struct S { char buf[32]; };
+template<S s> constexpr const char *begin() { return s.buf; }
+template<S s> constexpr const char *end() { return s.buf + __builtin_strlen(s.buf); }
+template<S s> constexpr const void *retval() { return &s; }
+extern const void *callee(const S*);
+template<S s> constexpr const void* observable_addr() { return callee(&s); }
+
+// CHECK: [[HELLO:@_ZTAXtl1StlA32_cLc104ELc101ELc108ELc108ELc111ELc32ELc119ELc111ELc114ELc108ELc100EEEE]]
+// CHECK-SAME: = linkonce_odr addrspace(1) constant { <{ [11 x i8], [21 x i8] }> } { <{ [11 x i8], [21 x i8] }> <{ [11 x i8] c"hello world", [21 x i8] zeroinitializer }> }, comdat
+
+// CHECK: @p
+// CHECK-SAME: addrspace(1) global ptr addrspace(4) addrspacecast (ptr addrspace(1) [[HELLO]] to ptr addrspace(4))
+const char *p = begin<S{"hello world"}>();
+
+// CHECK: @q
+// CHECK-SAME: addrspace(1) global ptr addrspace(4) addrspacecast (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) [[HELLO]], i64 11) to ptr addrspace(4))
+const char *q = end<S{"hello world"}>();
+
+const void *(*r)() = &retval<S{"hello world"}>;
+
+// CHECK: @s
+// CHECK-SAME: addrspace(1) global ptr addrspace(4) null
+const void *s = observable_addr<S{"hello world"}>();
+
+// CHECK: define linkonce_odr {{.*}} noundef ptr addrspace(4) @_Z6retvalIXtl1StlA32_cLc104ELc101ELc108ELc108ELc111ELc32ELc119ELc111ELc114ELc108ELc100EEEEEPKvv()
+// CHECK: ret ptr addrspace(4) addrspacecast (ptr addrspace(1) [[HELLO]] to ptr addrspace(4))
+
+// CHECK: define linkonce_odr {{.*}} noundef ptr addrspace(4) @_Z15observable_addrIXtl1StlA32_cLc104ELc101ELc108ELc108ELc111ELc32ELc119ELc111ELc114ELc108ELc100EEEEEPKvv()
+// CHECK: %call = call {{.*}} noundef ptr addrspace(4) @_Z6calleePK1S(ptr addrspace(4) noundef addrspacecast (ptr addrspace(1) [[HELLO]] to ptr addrspace(4)))
+// CHECK: declare {{.*}} noundef ptr addrspace(4) @_Z6calleePK1S(ptr addrspace(4) noundef)
diff --git a/clang/test/CodeGenCXX/throw-expression-typeinfo-in-nonzero-default-address-space.cpp b/clang/test/CodeGenCXX/throw-expression-typeinfo-in-nonzero-default-address-space.cpp
new file mode 100644
index 00000000000000..b7fcde23efe6dd
--- /dev/null
+++ b/clang/test/CodeGenCXX/throw-expression-typeinfo-in-nonzero-default-address-space.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 %s -triple spirv64-unknown-unknown -fsycl-is-device -emit-llvm -fcxx-exceptions -fexceptions -std=c++11 -o - | FileCheck %s
+
+struct X {
+ ~X();
+};
+
+struct Error {
+ Error(const X&) noexcept;
+};
+
+void f() {
+ try {
+ throw Error(X());
+ } catch (...) { }
+}
+
+// CHECK: declare{{.*}} void @__cxa_throw(ptr addrspace(4), ptr addrspace(1), ptr addrspace(4))
diff --git a/clang/test/CodeGenCXX/try-catch-with-nonzero-default-address-space.cpp b/clang/test/CodeGenCXX/try-catch-with-nonzero-default-address-space.cpp
new file mode 100644
index 00000000000000..62143ef580835b
--- /dev/null
+++ b/clang/test/CodeGenCXX/try-catch-with-nonzero-default-address-space.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 %s -triple=spirv64-unknown-unknown -fsycl-is-device -emit-llvm -o - -fcxx-exceptions -fexceptions | FileCheck %s
+
+struct X { };
+
+const X g();
+
+void f() {
+ try {
+ throw g();
+ // CHECK: ptr addrspace(1) @_ZTI1X
+ } catch (const X x) {
+ // CHECK: catch ptr addrspace(1) @_ZTI1X
+ // CHECK: call i32 @llvm.eh.typeid.for(ptr addrspacecast (ptr addrspace(1) @_ZTI1X to ptr))
+ }
+}
+
+void h() {
+ try {
+ throw "ABC";
+ // CHECK: ptr addrspace(1) @_ZTIPKc
+ } catch (char const(&)[4]) {
+ // CHECK: catch ptr addrspace(1) @_ZTIA4_c
+ // CHECK: call i32 @llvm.eh.typeid.for(ptr addrspacecast (ptr addrspace(1) @_ZTIA4_c to ptr))
+ }
+}
diff --git a/clang/test/CodeGenCXX/typeid-cxx11-with-nonzero-default-address-space.cpp b/clang/test/CodeGenCXX/typeid-cxx11-with-nonzero-default-address-space.cpp
new file mode 100644
index 00000000000000..cc6bc7f53b70af
--- /dev/null
+++ b/clang/test/CodeGenCXX/typeid-cxx11-with-nonzero-default-address-space.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -I%S %s -triple spirv64-unknown-unknown -fsycl-is-device -emit-llvm -std=c++11 -o - | FileCheck %s
+#include "typeinfo"
+
+namespace Test1 {
+
+struct Item {
+ const std::type_info &ti;
+ const char *name;
+ void *(*make)();
+};
+
+template<typename T> void *make_impl() { return new T; }
+template<typename T> constexpr Item item(const char *name) {
+ return { typeid(T), name, make_impl<T> };
+}
+
+struct A { virtual ~A(); };
+struct B : virtual A {};
+struct C { int n; };
+
+// CHECK: @_ZN5Test15itemsE ={{.*}} addrspace(1) constant [4 x {{.*}}] [{{.*}} ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTIN5Test11AE to ptr addrspace(4)), {{.*}} @_ZN5Test19make_implINS_1AEEEPvv {{.*}} ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTIN5Test11BE to ptr addrspace(4)), {{.*}} @_ZN5Test19make_implINS_1BEEEPvv {{.*}} ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTIN5Test11CE to ptr addrspace(4)), {{.*}} @_ZN5Test19make_implINS_1CEEEPvv {{.*}} ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTIi to ptr addrspace(4)), {{.*}} @_ZN5Test19make_implIiEEPvv }]
+extern constexpr Item items[] = {
+ item<A>("A"), item<B>("B"), item<C>("C"), item<int>("int")
+};
+
+// CHECK: @_ZN5Test11xE ={{.*}} addrspace(1) constant ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTIN5Test11AE to ptr addrspace(4)), align 8
+constexpr auto &x = items[0].ti;
+
+// CHECK: @_ZN5Test11yE ={{.*}} addrspace(1) constant ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTIN5Test11BE to ptr addrspace(4)), align 8
+constexpr auto &y = typeid(B{});
+
+}
diff --git a/clang/test/CodeGenCXX/typeid-with-nonzero-default-address-space.cpp b/clang/test/CodeGenCXX/typeid-with-nonzero-default-address-space.cpp
new file mode 100644
index 00000000000000..3a5a5515c46b4e
--- /dev/null
+++ b/clang/test/CodeGenCXX/typeid-with-nonzero-default-address-space.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -I%S %s -triple spirv64-unknown-unknown -fsycl-is-device -emit-llvm -fcxx-exceptions -fexceptions -o - | FileCheck %s
+#include <typeinfo>
+
+namespace Test1 {
+
+// PR7400
+struct A { virtual void f(); };
+
+// CHECK: @_ZN5Test16int_tiE ={{.*}} constant ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTIi to ptr addrspace(4)), align 8
+const std::type_info &int_ti = typeid(int);
+
+// CHECK: @_ZN5Test14A_tiE ={{.*}} constant ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTIN5Test11AE to ptr addrspace(4)), align 8
+const std::type_info &A_ti = typeid(const volatile A &);
+
+volatile char c;
+
+// CHECK: @_ZN5Test14c_tiE ={{.*}} constant ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTIc to ptr addrspace(4)), align 8
+const std::type_info &c_ti = typeid(c);
+
+extern const double &d;
+
+// CHECK: @_ZN5Test14d_tiE ={{.*}} constant ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTId to ptr addrspace(4)), align 8
+const std::type_info &d_ti = typeid(d);
+
+extern A &a;
+
+// CHECK: @_ZN5Test14a_tiE ={{.*}} global
+const std::type_info &a_ti = typeid(a);
+
+// CHECK: @_ZN5Test18A10_c_tiE ={{.*}} constant ptr addrspace(4) addrspacecast (ptr addrspace(1) @_ZTIA10_c to ptr addrspace(4)), align 8
+const std::type_info &A10_c_ti = typeid(char const[10]);
+
+// CHECK-LABEL: define{{.*}} ptr addrspace(4) @_ZN5Test11fEv
+// CHECK-SAME: personality ptr @__gxx_personality_v0
+const char *f() {
+ try {
+ // CHECK: br i1
+ // CHECK: invoke{{.*}} void @__cxa_bad_typeid() [[NR:#[0-9]+]]
+ return typeid(*static_cast<A *>(0)).name();
+ } catch (...) {
+ // CHECK: landingpad { ptr addrspace(4), i32 }
+ // CHECK-NEXT: catch ptr addrspace(4) null
+ }
+
+ return 0;
+}
+
+}
+
+// CHECK: attributes [[NR]] = { noreturn }
diff --git a/clang/test/CodeGenCXX/typeinfo-with-nonzero-default-address-space.cpp b/clang/test/CodeGenCXX/typeinfo-with-nonzero-default-address-space.cpp
new file mode 100644
index 00000000000000..7eab1ba86b9dfe
--- /dev/null
+++ b/clang/test/CodeGenCXX/typeinfo-with-nonzero-default-address-space.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -I%S %s -triple spirv64-unknown-unknown -fsycl-is-device -emit-llvm -o - | FileCheck %s -check-prefix=AS
+// RUN: %clang_cc1 -I%S %s -triple x86_64-linux-gnu -emit-llvm -o - | FileCheck %s -check-prefix=NO-AS
+#include <typeinfo>
+
+class A {
+ virtual void f() = 0;
+};
+
+class B : A {
+ void f() override;
+};
+
+// AS: @_ZTISt9type_info = external addrspace(1) constant ptr addrspace(1)
+// NO-AS: @_ZTISt9type_info = external constant ptr
+// AS: @_ZTIi = external addrspace(1) constant ptr addrspace(1)
+// NO-AS: @_ZTIi = external constant ptr
+// AS: @_ZTVN10__cxxabiv117__class_type_infoE = external addrspace(1) global [0 x ptr addrspace(1)]
+// NO-AS: @_ZTVN10__cxxabiv117__class_type_infoE = external global [0 x ptr]
+// AS: @_ZTS1A = linkonce_odr addrspace(1) constant [3 x i8] c"1A\00", comdat, align 1
+// NO-AS: @_ZTS1A = linkonce_odr constant [3 x i8] c"1A\00", comdat, align 1
+// AS: @_ZTI1A = linkonce_odr addrspace(1) constant { ptr addrspace(1), ptr addrspace(1) } { ptr addrspace(1) getelementptr inbounds (ptr addrspace(1), ptr addrspace(1) @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), ptr addrspace(1) @_ZTS1A }, comdat, align 8
+// NO-AS: @_ZTI1A = linkonce_odr constant { ptr, ptr } { ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), ptr @_ZTS1A }, comdat, align 8
+// AS: @_ZTIf = external addrspace(1) constant ptr addrspace(1)
+// NO-AS: @_ZTIf = external constant ptr
+
+unsigned long Fn(B& b) {
+// AS: %call = call{{.*}} noundef zeroext i1 @_ZNKSt9type_infoeqERKS_(ptr addrspace(4) {{.*}} addrspacecast (ptr addrspace(1) @_ZTISt9type_info to ptr addrspace(4)), ptr addrspace(4) {{.*}} %2)
+// NO-AS: %call = call noundef zeroext i1 @_ZNKSt9type_infoeqERKS_(ptr {{.*}} @_ZTISt9type_info, ptr {{.*}} %2)
+ if (typeid(std::type_info) == typeid(b))
+ return 42;
+// AS: %call2 = call{{.*}} noundef zeroext i1 @_ZNKSt9type_infoneERKS_(ptr addrspace(4) {{.*}} addrspacecast (ptr addrspace(1) @_ZTIi to ptr addrspace(4)), ptr addrspace(4) {{.*}} %5)
+// NO-AS: %call2 = call noundef zeroext i1 @_ZNKSt9type_infoneERKS_(ptr {{.*}} @_ZTIi, ptr {{.*}} %5)
+ if (typeid(int) != typeid(b))
+ return 1712;
+// AS: %call5 = call{{.*}} noundef ptr addrspace(4) @_ZNKSt9type_info4nameEv(ptr addrspace(4) {{.*}} addrspacecast (ptr addrspace(1) @_ZTI1A to ptr addrspace(4)))
+// NO-AS: %call5 = call noundef ptr @_ZNKSt9type_info4nameEv(ptr {{.*}} @_ZTI1A)
+// AS: %call7 = call{{.*}} noundef ptr addrspace(4) @_ZNKSt9type_info4nameEv(ptr addrspace(4) {{.*}} %8)
+// NO-AS: %call7 = call noundef ptr @_ZNKSt9type_info4nameEv(ptr {{.*}} %8)
+ if (typeid(A).name() == typeid(b).name())
+ return 0;
+// AS: %call11 = call{{.*}} noundef zeroext i1 @_ZNKSt9type_info6beforeERKS_(ptr addrspace(4) {{.*}} %11, ptr addrspace(4) {{.*}} addrspacecast (ptr addrspace(1) @_ZTIf to ptr addrspace(4)))
+// NO-AS: %call11 = call noundef zeroext i1 @_ZNKSt9type_info6beforeERKS_(ptr {{.*}} %11, ptr {{.*}} @_ZTIf)
+ if (typeid(b).before(typeid(float)))
+ return 1;
+// AS: %call15 = call{{.*}} noundef i64 @_ZNKSt9type_info9hash_codeEv(ptr addrspace(4) {{.*}} %14)
+// NO-AS: %call15 = call noundef i64 @_ZNKSt9type_info9hash_codeEv(ptr {{.*}} %14)
+ return typeid(b).hash_code();
+}
diff --git a/clang/test/CodeGenCXX/vtable-align-nonzero-default-address-space.cpp b/clang/test/CodeGenCXX/vtable-align-nonzero-default-address-space.cpp
new file mode 100644
index 00000000000000..2a1a7293982f18
--- /dev/null
+++ b/clang/test/CodeGenCXX/vtable-align-nonzero-default-address-space.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 %s -triple=spirv64-unknown-unknown -fsycl-is-device -std=c++11...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
That is a bug in those DataLayouts |
I do think this should likely be part of the DataLayout. We have defaults for globals, alloca and functions, but no generic "give me the address space for |
|
Why can't we just declare that the "generic" address-space must always be 0? The specific numbers we use for address-spaces are completely arbitrary anyway. |
| @@ -0,0 +1,17 @@ | |||
| // RUN: %clang_cc1 %s -triple spirv64-unknown-unknown -fsycl-is-device -emit-llvm -fcxx-exceptions -fexceptions -std=c++11 -o - | FileCheck %s | |||
There was a problem hiding this comment.
Instead of duplicating all these files, could you add one additional RUN: line to the test you copied this from? No need to regenerate with update_cc_test_checks, just add a new check line with something like a CHECK-NONZERO-AS prefix?
…all_defaults_are_zero
…all_defaults_are_zero
clang/test/CodeGenCXX/vtable-assume-load-nonzero-default-address-space.cpp
Outdated
Show resolved
Hide resolved
clang/test/CodeGenCXX/vtable-assume-load-nonzero-default-address-space.cpp
Outdated
Show resolved
Hide resolved
…all_defaults_are_zero
…all_defaults_are_zero
…all_defaults_are_zero
| // RUN: FileCheck --check-prefix=CHECK6 --input-file=%t.ll %s | ||
| // RUN: FileCheck --check-prefix=CHECK7 --input-file=%t.ll %s | ||
| // RUN: FileCheck --check-prefix=CHECK8 --input-file=%t.ll %s | ||
| // RUN: FileCheck --check-prefix=CHECK9 --input-file=%t.ll %s |
There was a problem hiding this comment.
Why renumber everything? Just add yours to the end?
There was a problem hiding this comment.
Ah, the renumbering is a consequence of merging in the base vtable-assume-load.cpp test; initially when I "wrote" this one, I had removed the CHECK-MS case, which just made it look awkward when compared with the base, as @arichardson pointed out in another comment, so I merely brought that back in so that it looks less odd when comparing with the base case. Apologies for the noise.
There was a problem hiding this comment.
Maybe it would be easier to autogenerate these check lines and just have the RUN: lines be:
// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - -O1 -disable-llvm-passes -fms-extensions -fstrict-vtable-pointers | FileCheck %s --check-prefix=CHECK-AMDGCN
// RUN: %clang_cc1 %s -triple i686-pc-win32 -emit-llvm -o - -O1 -disable-llvm-passes -fms-extensions -fstrict-vtable-pointers | FileCheck %s --check-prefix=CHECK-MS
// RUN: %clang_cc1 %s -triple=spirv64-unknown-unknown -fsycl-is-device -std=c++11 -emit-llvm -o %t.ll -O1 -disable-llvm-passes -fms-extensions -fstrict-vtable-pointers | FileCheck %s --check-prefix=CHECK-SPIRV
| @@ -23,8 +26,8 @@ struct B : A { | |||
| void g(A *a) { a->foo(); } | |||
|
|
|||
| // CHECK1-LABEL: define{{.*}} void @_ZN5test14fooAEv() | |||
| // CHECK1: call void @_ZN5test11AC1Ev(ptr | |||
| // CHECK1: %[[VTABLE:.*]] = load ptr addrspace(1), ptr %{{.*}} | |||
| // CHECK1: call{{.*}} void @_ZN5test11AC1Ev(ptr {{((addrspace(4)){0,1})}} | |||
There was a problem hiding this comment.
I don’t love how this isn’t checking the address space is correct for each, only that it’s one of the two valid ones
There was a problem hiding this comment.
Hmm, I can rework the regexp (it would still be icky), but it's not going to help with the issue I don't think. I would prefer to not spam another prefix, which is the only other way I can see for dealing with this without duplicating the test, but if the lack of love is extreme I can probably go that way.
There was a problem hiding this comment.
I'd say this is probably fine for now, but these tests should be deduplicated and cleaned up in a follow-up.
There was a problem hiding this comment.
Right, so what I'll do is: merge this as is, and then revisit the test and rework it along the lines you and @jrtc27 are suggesting, since it looks as if I'll need to mess with it again for another piece of work.
…all_defaults_are_zero
…all_defaults_are_zero
…all_defaults_are_zero
There was a problem hiding this comment.
Please fix the commit message before you merge; otherwise LGTM
For the commit message I was thinking something along the following lines: At the moment, Clang is rather liberal in assuming that 0 (and by extension unqualified) is always a safe default. This does not work for targets that actually use a different value for the default / generic AS (for example, the SPIRV that obtains from HIPSPV or SYCL). This patch is a first, fairly safe step towards trying to clear things up by querying a modules default AS from the target, rather than assuming it's 0, alongside fixing a few places where things break / we encode the 0 AS assumption. A bunch of existing tests are extended to check for non-zero default AS usage.
…all_defaults_are_zero
…all_defaults_are_zero
| @@ -368,7 +368,8 @@ CodeGenModule::CodeGenModule(ASTContext &C, | |||
| IntTy = llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getIntWidth()); | |||
| IntPtrTy = llvm::IntegerType::get(LLVMContext, | |||
| C.getTargetInfo().getMaxPointerWidth()); | |||
| Int8PtrTy = llvm::PointerType::get(LLVMContext, 0); | |||
| Int8PtrTy = llvm::PointerType::get(LLVMContext, | |||
There was a problem hiding this comment.
This seems to cause regressions for HIP -fsanitize=address.
Basically rocm device library for ASAN is compiled from OpenCL source code, for which the default address space is mapped to addr space 5 in IR. Int8PtrTy is used to determine the pointer type in the llvm.compiler.used global array, which causes the pointers in that array to be in addr space 5. However, for HIP those are in addr space 0. The variable from different bitcode are appended together by the linker. The linker checks their type and emits error since they do not match.
https://godbolt.org/z/s48fTj7Pv
Before this change, the pointers are in addr space 0 for both HIP and OpenCL, therefore no such issue.
There was a problem hiding this comment.
This seems to be an error on ASAN's side, it's relying on non-guaranteed behaviour and linking BC emanating from different languages with different AS maps. To wit, 0 is private for OCL, so having llvm.used and llvm.compiler.used be global arrays of pointers to private is somewhat suspect (especially since useds are Globals). I will / should fix emitUsed to rely on GlobalsInt8PtrTy, which seems like the right thing to do anyway, but whilst that'll make ASAN "work", it doesn't make ASAN correct, IMHO.
There was a problem hiding this comment.
The AMDGPU address space mapping is at https://llvm.org/docs/AMDGPUUsage.html#address-spaces and address space 0 is not private. The address space for all languages compiled for the AMDGPU target is consistent.
There was a problem hiding this comment.
The IR address space is a pure target concept. Any address space error would be on the frontend emitting the wrong IR for the target
There was a problem hiding this comment.
Fixing emitUsed probably is a feasible solution, but GlobalsInt8PtrTy will cause the used array containing pointers to addr space 1, which is not backward compatible for HIP. Maybe just use pointer to addr space 0 type as before.
There was a problem hiding this comment.
I believe there is special case linker code to handle mismatched address spaces for the intrinsic globals, so I don't think changing this would break anything. The address space of llvm.used doesn't really mean anything
There was a problem hiding this comment.
Fixing emitUsed probably is a feasible solution, but GlobalsInt8PtrTy will cause the used array containing pointers to addr space 1, which is not backward compatible for HIP. Maybe just use pointer to addr space 0 type as before.
This would be wrong, assuming that 0 / Unqual is generally equivalent with some form of generic pointer is the core problem here. I'm not exactly sure what you mean by "backward compatible" here though, both the HIP source and whatever BC gets linked should go through the same toolchain, surely? Also, what @arsenm said sounds about right.
There was a problem hiding this comment.
The IR address space is a pure target concept. Any address space error would be on the frontend emitting the wrong IR for the target
I don't think mixing languages with different LangAS maps is sound, OCL originating BC is not generic BC. There's no "error" here, the FE is doing what is asked of it (indiscriminately assuming that AS 0 is some generic/flat pointer that is valid everywhere is rather risque, as per prior conversation in this review). The actual problem is, AFAICT, that when we call AMDGPUTargetInfo::adjust we indiscriminately set private as default for any and all OCL compilations? There's a 6 year old TODO, which may or may not be vestigial at this point.
There was a problem hiding this comment.
I don't think mixing languages with different LangAS maps is sound
It is entirely sound, and required for this entire system to work (e.g. we implement the libraries in OpenCL used by all the languages). The point of the LangAS is to map to the target address space, which does not care what the language is. Different languages should be trying to be ABI compatible with each other, which includes emitting the correct IR address space
There was a problem hiding this comment.
I don't think mixing languages with different LangAS maps is sound
It is entirely sound, and required for this entire system to work
I am aware of what we decided to do. Whether or not that is actually sound is a different kettle of fish, but let's agree to disagree and move on. Stepping back, I'm trying to figure out what the actual suggestion is. Reverting the change is wrong, because there are other targets where for which 0 is definitely not a sound default, and this impacts more than the used/compiler.used arrays. Seems like there are two viable options:
- I'm going to propose changing
emitUsedanyway, since that makes sense even if the AS is indeed mostly meaningless there - IMHO OCL should default to
AMDGPUDefIsGenMapfor its LangAS map, which would "fix" this as well.
If there's some third reasonable solution, I apologise for missing it - please share!
|
Just noting here that all ASAN testing on the staging branch is blocked and other problems could creep in while it remains so. |
At the moment, Clang is rather liberal in assuming that 0 (and by extension unqualified) is always a safe default. This does not work for targets that actually use a different value for the default / generic AS (for example, the SPIRV that obtains from HIPSPV or SYCL). This patch is a first step, fairly safe step towards trying to clear things up by:
GlobalVariables /Functions that take an address space argument, as opposed to the defaults that assume 0.A bunch of tests (adapted from existing ones) are added. I've opted against adding new cases within to the existing ones sinc e some are fairly verbose already.