[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
This revision was automatically updated to reflect the committed changes.
Closed by commit rL366059: [OpenCL][PR41727] Prevent ICE on global dtors
(authored by stulova, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D62413?vs=209463&id=209804#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62413/new/
https://reviews.llvm.org/D62413
Files:
cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/TargetInfo.h
cfe/trunk/test/CodeGenOpenCLCXX/atexit.cl
Index: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
===
--- cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
+++ cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
@@ -2284,8 +2284,19 @@
llvm::Type *dtorTy =
llvm::FunctionType::get(CGF.VoidTy, CGF.Int8PtrTy, false)->getPointerTo();
+ // Preserve address space of addr.
+ auto AddrAS = addr ? addr->getType()->getPointerAddressSpace() : 0;
+ auto AddrInt8PtrTy =
+ AddrAS ? CGF.Int8Ty->getPointerTo(AddrAS) : CGF.Int8PtrTy;
+
+ // Create a variable that binds the atexit to this shared object.
+ llvm::Constant *handle =
+ CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
+ auto *GV = cast(handle->stripPointerCasts());
+ GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
// extern "C" int __cxa_atexit(void (*f)(void *), void *p, void *d);
- llvm::Type *paramTys[] = { dtorTy, CGF.Int8PtrTy, CGF.Int8PtrTy };
+ llvm::Type *paramTys[] = {dtorTy, AddrInt8PtrTy, handle->getType()};
llvm::FunctionType *atexitTy =
llvm::FunctionType::get(CGF.IntTy, paramTys, false);
@@ -2294,12 +2305,6 @@
if (llvm::Function *fn = dyn_cast(atexit.getCallee()))
fn->setDoesNotThrow();
- // Create a variable that binds the atexit to this shared object.
- llvm::Constant *handle =
- CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
- auto *GV = cast(handle->stripPointerCasts());
- GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
-
if (!addr)
// addr is null when we are trying to register a dtor annotated with
// __attribute__((destructor)) in a constructor function. Using null here is
@@ -2309,7 +2314,7 @@
llvm::Value *args[] = {llvm::ConstantExpr::getBitCast(
cast(dtor.getCallee()), dtorTy),
- llvm::ConstantExpr::getBitCast(addr, CGF.Int8PtrTy),
+ llvm::ConstantExpr::getBitCast(addr, AddrInt8PtrTy),
handle};
CGF.EmitNounwindRuntimeCall(atexit, args);
}
Index: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp
@@ -3577,8 +3577,12 @@
llvm::Constant *
CodeGenModule::CreateRuntimeVariable(llvm::Type *Ty,
StringRef Name) {
- auto *Ret =
- GetOrCreateLLVMGlobal(Name, llvm::PointerType::getUnqual(Ty), nullptr);
+ auto PtrTy =
+ getContext().getLangOpts().OpenCL
+ ? llvm::PointerType::get(
+Ty, getContext().getTargetAddressSpace(LangAS::opencl_global))
+ : llvm::PointerType::getUnqual(Ty);
+ auto *Ret = GetOrCreateLLVMGlobal(Name, PtrTy, nullptr);
setDSOLocal(cast(Ret->stripPointerCasts()));
return Ret;
}
Index: cfe/trunk/lib/CodeGen/TargetInfo.h
===
--- cfe/trunk/lib/CodeGen/TargetInfo.h
+++ cfe/trunk/lib/CodeGen/TargetInfo.h
@@ -267,6 +267,11 @@
LangAS SrcAddr, LangAS DestAddr,
llvm::Type *DestTy) const;
+ /// Get address space of pointer parameter for __cxa_atexit.
+ virtual LangAS getAddrSpaceOfCxaAtexitPtrParam() const {
+return LangAS::Default;
+ }
+
/// Get the syncscope used in LLVM IR.
virtual llvm::SyncScope::ID getLLVMSyncScopeID(const LangOptions &LangOpts,
SyncScope Scope,
Index: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
===
--- cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
+++ cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
@@ -14,6 +14,7 @@
#include "CGCXXABI.h"
#include "CGObjCRuntime.h"
#include "CGOpenMPRuntime.h"
+#include "TargetInfo.h"
#include "clang/Basic/CodeGenOptions.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/Intrinsics.h"
@@ -118,9 +119,22 @@
CXXDestructorDecl *Dtor = Record->getDestructor();
Func = CGM.getAddrAndTypeOfCXXStructor(GlobalDecl(Dtor, Dtor_Complete));
-Argument = llvm::ConstantExpr::getBitCast(
-Addr.getPointer(), CGF.getTypes().ConvertType(Type)->getPointerTo());
-
+if (CGF.getContext().getLangOpts().OpenCL)
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
rjmccall added a comment. If you're interested in working on this, great. I actually think there's zero reason to emit a non-null argument here on any target unless we're going to use the destructor as the function pointer — but we can do that on every Itanium target, so we should. So where we want to be is probably: - use the complete-object destructor and the object pointer as its argument when destroying a (non-array) object of C++ class type - otherwise use a custom function that materializes the object pointer on its own and uses a null pointer as the argument And then the address-spaces tweak to that is that we use the second option when we either (1) the destructor isn't in the `__cxa_atexit` address space or (2) the object pointer can't be converted to that address space. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Current patch LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
Anastasia updated this revision to Diff 209463.
Anastasia added a comment.
Generate NULL for pointer param of __cxa_atexit on mismatching addr spaces
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62413/new/
https://reviews.llvm.org/D62413
Files:
lib/CodeGen/CGDeclCXX.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/ItaniumCXXABI.cpp
lib/CodeGen/TargetInfo.h
test/CodeGenOpenCLCXX/atexit.cl
Index: test/CodeGenOpenCLCXX/atexit.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/atexit.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_cc1 %s -triple spir -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct S {
+ ~S(){};
+};
+S s;
+
+//CHECK-LABEL: define internal void @__cxx_global_var_init()
+//CHECK: call i32 @__cxa_atexit(void (i8*)* bitcast (void (%struct.S addrspace(4)*)* @_ZNU3AS41SD1Ev to void (i8*)*), i8* null, i8 addrspace(1)* @__dso_handle)
+
+//CHECK: declare i32 @__cxa_atexit(void (i8*)*, i8*, i8 addrspace(1)*)
Index: lib/CodeGen/TargetInfo.h
===
--- lib/CodeGen/TargetInfo.h
+++ lib/CodeGen/TargetInfo.h
@@ -267,6 +267,11 @@
LangAS SrcAddr, LangAS DestAddr,
llvm::Type *DestTy) const;
+ /// Get address space of pointer parameter for __cxa_atexit.
+ virtual LangAS getAddrSpaceOfCxaAtexitPtrParam() const {
+return LangAS::Default;
+ }
+
/// Get the syncscope used in LLVM IR.
virtual llvm::SyncScope::ID getLLVMSyncScopeID(const LangOptions &LangOpts,
SyncScope Scope,
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -2284,8 +2284,19 @@
llvm::Type *dtorTy =
llvm::FunctionType::get(CGF.VoidTy, CGF.Int8PtrTy, false)->getPointerTo();
+ // Preserve address space of addr.
+ auto AddrAS = addr ? addr->getType()->getPointerAddressSpace() : 0;
+ auto AddrInt8PtrTy =
+ AddrAS ? CGF.Int8Ty->getPointerTo(AddrAS) : CGF.Int8PtrTy;
+
+ // Create a variable that binds the atexit to this shared object.
+ llvm::Constant *handle =
+ CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
+ auto *GV = cast(handle->stripPointerCasts());
+ GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
// extern "C" int __cxa_atexit(void (*f)(void *), void *p, void *d);
- llvm::Type *paramTys[] = { dtorTy, CGF.Int8PtrTy, CGF.Int8PtrTy };
+ llvm::Type *paramTys[] = {dtorTy, AddrInt8PtrTy, handle->getType()};
llvm::FunctionType *atexitTy =
llvm::FunctionType::get(CGF.IntTy, paramTys, false);
@@ -2294,12 +2305,6 @@
if (llvm::Function *fn = dyn_cast(atexit.getCallee()))
fn->setDoesNotThrow();
- // Create a variable that binds the atexit to this shared object.
- llvm::Constant *handle =
- CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
- auto *GV = cast(handle->stripPointerCasts());
- GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
-
if (!addr)
// addr is null when we are trying to register a dtor annotated with
// __attribute__((destructor)) in a constructor function. Using null here is
@@ -2309,7 +2314,7 @@
llvm::Value *args[] = {llvm::ConstantExpr::getBitCast(
cast(dtor.getCallee()), dtorTy),
- llvm::ConstantExpr::getBitCast(addr, CGF.Int8PtrTy),
+ llvm::ConstantExpr::getBitCast(addr, AddrInt8PtrTy),
handle};
CGF.EmitNounwindRuntimeCall(atexit, args);
}
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -3577,8 +3577,12 @@
llvm::Constant *
CodeGenModule::CreateRuntimeVariable(llvm::Type *Ty,
StringRef Name) {
- auto *Ret =
- GetOrCreateLLVMGlobal(Name, llvm::PointerType::getUnqual(Ty), nullptr);
+ auto PtrTy =
+ getContext().getLangOpts().OpenCL
+ ? llvm::PointerType::get(
+Ty, getContext().getTargetAddressSpace(LangAS::opencl_global))
+ : llvm::PointerType::getUnqual(Ty);
+ auto *Ret = GetOrCreateLLVMGlobal(Name, PtrTy, nullptr);
setDSOLocal(cast(Ret->stripPointerCasts()));
return Ret;
}
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -14,6 +14,7 @@
#include "CGCXXABI.h"
#include "CGObjCRuntime.h"
#include "CGOpenMPRuntime.h"
+#include "TargetInfo.h"
#include "clang/Basic/CodeGenOptions.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/Intrinsics.h"
@@ -118,9 +119,22 @@
CXXDestructorDecl *Dtor = Record->getDestructor();
Func = CGM.getAddrAndTypeOfCXXStructor(Glo
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
Anastasia marked an inline comment as done. Anastasia added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); rjmccall wrote: > Anastasia wrote: > > rjmccall wrote: > > > rjmccall wrote: > > > > Anastasia wrote: > > > > > rjmccall wrote: > > > > > > Anastasia wrote: > > > > > > > rjmccall wrote: > > > > > > > > Should this code be conditional to OpenCL? And why does > > > > > > > > `_cxa_atexit` take a `__global` pointer instead of, say, a > > > > > > > > `__generic` one? > > > > > > > The only objects that are destructible globally in OpenCL are > > > > > > > `__global` and `__constant`. However `__constant` isn't > > > > > > > convertible to `__generic`. Therefore, I am adding `__global` > > > > > > > directly to avoid extra conversion. I am not yet sure how to > > > > > > > handle `__constant`though and how much destructing objects in > > > > > > > read-only memory segments would make sense anyway. I think I will > > > > > > > address this separately. > > > > > > The pointer argument to `__cxa_atexit` is just an arbitrary bit of > > > > > > context and doesn't have to actually be the address of a global. > > > > > > It's *convenient* to use the address of a global sometimes; e.g. > > > > > > you can use the global as the pointer and its destructor as the > > > > > > function, and then `__cxa_atexit` will just call the destructor for > > > > > > you without any additional code. But as far as the runtime is > > > > > > concerned, the pointer could be `malloc`'ed or something; we've > > > > > > never had a need to do that in the ABI, but it's good > > > > > > future-proofing to allow it. > > > > > > > > > > > > So there are three ways to get a global destructor to destroy a > > > > > > variable in `__constant`: > > > > > > - You can pass the pointer bitcast'ed as long as `sizeof(__constant > > > > > > void*) <= sizeof(__cxa_atexit_context_pointer)`. > > > > > > - You can ignore the argument and just materialize the address > > > > > > separately within the destructor function. > > > > > > - You can allocate memory for a context and then store the pointer > > > > > > in that. > > > > > > > > > > > > Obviously you should go with the one of the first two, but you > > > > > > should make sure your ABI doesn't preclude doing the latter in case > > > > > > it's useful for some future language feature. In other words, it > > > > > > doesn't really matter whether this argument is notionally in > > > > > > `__global` as long as that's wide enough to pass a more-or-less > > > > > > arbitrary pointer through. > > > > > Ok, I see. I guess option 1 would be fine since we can't setup > > > > > pointer width per address space in clang currently. However, spec > > > > > doesn't provide any clarifications in this regard. > > > > > > > > > > So I guess using either `__global` or `__generic` for the pointer > > > > > parameter would be fine... Or perhaps even leave it without any > > > > > address space (i.e. _`_private`) and just addrspacecast from either > > > > > `__global` or `__constant`. Do you have any preferences? > > > > > > > > > > As for `malloc` I am not sure that will work for OpenCL since we > > > > > don't allow mem allocation on the device. Unless you mean the memory > > > > > is allocated on a host... then I am not sure how it should work. > > > > > Ok, I see. I guess option 1 would be fine since we can't setup > > > > > pointer width per address space in clang currently. > > > > > > > > Really? What's missing there? It looks to me like `getPointerSize` > > > > does take an address space. > > > > > > > > > So I guess using either __global or __generic for the pointer > > > > > parameter would be fine... Or perhaps even leave it without any > > > > > address space (i.e. _`_private`) and just addrspacecast from either > > > > > __global or __constant. Do you have any preferences? > > > > > > > > `__private` is likely to be a smaller address space, right? I would > > > > recommend using the fattest pointer that you want to actually support > > > > at runtime — you shouldn't go all the way to `__generic` if the target > > > > relies on eliminating that statically. If you want a target hook for > > > > the address space of the notional `__cxa_atexit_context_pointer` > > > > typedef, I think that would be reasonable. > > > > > > > > > As for malloc I am not sure that will work for OpenCL since we don't > > > > > allow mem allocation on the device. Unless you mean the memory is > > > > > allocated on a host... then I am not sure how it should work. > > > > > > > > Well, maybe not actually heap-allocated. I just think you should > > > > design the ABI so that it's reasonably future-proof against taking any > > > > specific sort of reasonable pointer. > > > This cast on
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); Anastasia wrote: > rjmccall wrote: > > rjmccall wrote: > > > Anastasia wrote: > > > > rjmccall wrote: > > > > > Anastasia wrote: > > > > > > rjmccall wrote: > > > > > > > Should this code be conditional to OpenCL? And why does > > > > > > > `_cxa_atexit` take a `__global` pointer instead of, say, a > > > > > > > `__generic` one? > > > > > > The only objects that are destructible globally in OpenCL are > > > > > > `__global` and `__constant`. However `__constant` isn't convertible > > > > > > to `__generic`. Therefore, I am adding `__global` directly to avoid > > > > > > extra conversion. I am not yet sure how to handle > > > > > > `__constant`though and how much destructing objects in read-only > > > > > > memory segments would make sense anyway. I think I will address > > > > > > this separately. > > > > > The pointer argument to `__cxa_atexit` is just an arbitrary bit of > > > > > context and doesn't have to actually be the address of a global. > > > > > It's *convenient* to use the address of a global sometimes; e.g. you > > > > > can use the global as the pointer and its destructor as the function, > > > > > and then `__cxa_atexit` will just call the destructor for you without > > > > > any additional code. But as far as the runtime is concerned, the > > > > > pointer could be `malloc`'ed or something; we've never had a need to > > > > > do that in the ABI, but it's good future-proofing to allow it. > > > > > > > > > > So there are three ways to get a global destructor to destroy a > > > > > variable in `__constant`: > > > > > - You can pass the pointer bitcast'ed as long as `sizeof(__constant > > > > > void*) <= sizeof(__cxa_atexit_context_pointer)`. > > > > > - You can ignore the argument and just materialize the address > > > > > separately within the destructor function. > > > > > - You can allocate memory for a context and then store the pointer in > > > > > that. > > > > > > > > > > Obviously you should go with the one of the first two, but you should > > > > > make sure your ABI doesn't preclude doing the latter in case it's > > > > > useful for some future language feature. In other words, it doesn't > > > > > really matter whether this argument is notionally in `__global` as > > > > > long as that's wide enough to pass a more-or-less arbitrary pointer > > > > > through. > > > > Ok, I see. I guess option 1 would be fine since we can't setup pointer > > > > width per address space in clang currently. However, spec doesn't > > > > provide any clarifications in this regard. > > > > > > > > So I guess using either `__global` or `__generic` for the pointer > > > > parameter would be fine... Or perhaps even leave it without any address > > > > space (i.e. _`_private`) and just addrspacecast from either `__global` > > > > or `__constant`. Do you have any preferences? > > > > > > > > As for `malloc` I am not sure that will work for OpenCL since we don't > > > > allow mem allocation on the device. Unless you mean the memory is > > > > allocated on a host... then I am not sure how it should work. > > > > Ok, I see. I guess option 1 would be fine since we can't setup pointer > > > > width per address space in clang currently. > > > > > > Really? What's missing there? It looks to me like `getPointerSize` does > > > take an address space. > > > > > > > So I guess using either __global or __generic for the pointer parameter > > > > would be fine... Or perhaps even leave it without any address space > > > > (i.e. _`_private`) and just addrspacecast from either __global or > > > > __constant. Do you have any preferences? > > > > > > `__private` is likely to be a smaller address space, right? I would > > > recommend using the fattest pointer that you want to actually support at > > > runtime — you shouldn't go all the way to `__generic` if the target > > > relies on eliminating that statically. If you want a target hook for the > > > address space of the notional `__cxa_atexit_context_pointer` typedef, I > > > think that would be reasonable. > > > > > > > As for malloc I am not sure that will work for OpenCL since we don't > > > > allow mem allocation on the device. Unless you mean the memory is > > > > allocated on a host... then I am not sure how it should work. > > > > > > Well, maybe not actually heap-allocated. I just think you should design > > > the ABI so that it's reasonably future-proof against taking any specific > > > sort of reasonable pointer. > > This cast only works if the address space is a subspace of the > > `__cxa_atexit` address space, right? Should we be checking that and > > emitting a diagnostic if that's not true? I think an IRGen-level > > diagnostic is fine here. > Then
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
Anastasia marked an inline comment as done. Anastasia added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); rjmccall wrote: > rjmccall wrote: > > Anastasia wrote: > > > rjmccall wrote: > > > > Anastasia wrote: > > > > > rjmccall wrote: > > > > > > Should this code be conditional to OpenCL? And why does > > > > > > `_cxa_atexit` take a `__global` pointer instead of, say, a > > > > > > `__generic` one? > > > > > The only objects that are destructible globally in OpenCL are > > > > > `__global` and `__constant`. However `__constant` isn't convertible > > > > > to `__generic`. Therefore, I am adding `__global` directly to avoid > > > > > extra conversion. I am not yet sure how to handle `__constant`though > > > > > and how much destructing objects in read-only memory segments would > > > > > make sense anyway. I think I will address this separately. > > > > The pointer argument to `__cxa_atexit` is just an arbitrary bit of > > > > context and doesn't have to actually be the address of a global. It's > > > > *convenient* to use the address of a global sometimes; e.g. you can use > > > > the global as the pointer and its destructor as the function, and then > > > > `__cxa_atexit` will just call the destructor for you without any > > > > additional code. But as far as the runtime is concerned, the pointer > > > > could be `malloc`'ed or something; we've never had a need to do that in > > > > the ABI, but it's good future-proofing to allow it. > > > > > > > > So there are three ways to get a global destructor to destroy a > > > > variable in `__constant`: > > > > - You can pass the pointer bitcast'ed as long as `sizeof(__constant > > > > void*) <= sizeof(__cxa_atexit_context_pointer)`. > > > > - You can ignore the argument and just materialize the address > > > > separately within the destructor function. > > > > - You can allocate memory for a context and then store the pointer in > > > > that. > > > > > > > > Obviously you should go with the one of the first two, but you should > > > > make sure your ABI doesn't preclude doing the latter in case it's > > > > useful for some future language feature. In other words, it doesn't > > > > really matter whether this argument is notionally in `__global` as long > > > > as that's wide enough to pass a more-or-less arbitrary pointer through. > > > Ok, I see. I guess option 1 would be fine since we can't setup pointer > > > width per address space in clang currently. However, spec doesn't provide > > > any clarifications in this regard. > > > > > > So I guess using either `__global` or `__generic` for the pointer > > > parameter would be fine... Or perhaps even leave it without any address > > > space (i.e. _`_private`) and just addrspacecast from either `__global` or > > > `__constant`. Do you have any preferences? > > > > > > As for `malloc` I am not sure that will work for OpenCL since we don't > > > allow mem allocation on the device. Unless you mean the memory is > > > allocated on a host... then I am not sure how it should work. > > > Ok, I see. I guess option 1 would be fine since we can't setup pointer > > > width per address space in clang currently. > > > > Really? What's missing there? It looks to me like `getPointerSize` does > > take an address space. > > > > > So I guess using either __global or __generic for the pointer parameter > > > would be fine... Or perhaps even leave it without any address space (i.e. > > > _`_private`) and just addrspacecast from either __global or __constant. > > > Do you have any preferences? > > > > `__private` is likely to be a smaller address space, right? I would > > recommend using the fattest pointer that you want to actually support at > > runtime — you shouldn't go all the way to `__generic` if the target relies > > on eliminating that statically. If you want a target hook for the address > > space of the notional `__cxa_atexit_context_pointer` typedef, I think that > > would be reasonable. > > > > > As for malloc I am not sure that will work for OpenCL since we don't > > > allow mem allocation on the device. Unless you mean the memory is > > > allocated on a host... then I am not sure how it should work. > > > > Well, maybe not actually heap-allocated. I just think you should design > > the ABI so that it's reasonably future-proof against taking any specific > > sort of reasonable pointer. > This cast only works if the address space is a subspace of the `__cxa_atexit` > address space, right? Should we be checking that and emitting a diagnostic > if that's not true? I think an IRGen-level diagnostic is fine here. Then it would fail to compile for `__constant`. > You can pass the pointer bitcast'ed as long as sizeof(__constant void*) <= > sizeof(__cxa_atexit_context_poin
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); rjmccall wrote: > Anastasia wrote: > > rjmccall wrote: > > > Anastasia wrote: > > > > rjmccall wrote: > > > > > Should this code be conditional to OpenCL? And why does > > > > > `_cxa_atexit` take a `__global` pointer instead of, say, a > > > > > `__generic` one? > > > > The only objects that are destructible globally in OpenCL are > > > > `__global` and `__constant`. However `__constant` isn't convertible to > > > > `__generic`. Therefore, I am adding `__global` directly to avoid extra > > > > conversion. I am not yet sure how to handle `__constant`though and how > > > > much destructing objects in read-only memory segments would make sense > > > > anyway. I think I will address this separately. > > > The pointer argument to `__cxa_atexit` is just an arbitrary bit of > > > context and doesn't have to actually be the address of a global. It's > > > *convenient* to use the address of a global sometimes; e.g. you can use > > > the global as the pointer and its destructor as the function, and then > > > `__cxa_atexit` will just call the destructor for you without any > > > additional code. But as far as the runtime is concerned, the pointer > > > could be `malloc`'ed or something; we've never had a need to do that in > > > the ABI, but it's good future-proofing to allow it. > > > > > > So there are three ways to get a global destructor to destroy a variable > > > in `__constant`: > > > - You can pass the pointer bitcast'ed as long as `sizeof(__constant > > > void*) <= sizeof(__cxa_atexit_context_pointer)`. > > > - You can ignore the argument and just materialize the address separately > > > within the destructor function. > > > - You can allocate memory for a context and then store the pointer in > > > that. > > > > > > Obviously you should go with the one of the first two, but you should > > > make sure your ABI doesn't preclude doing the latter in case it's useful > > > for some future language feature. In other words, it doesn't really > > > matter whether this argument is notionally in `__global` as long as > > > that's wide enough to pass a more-or-less arbitrary pointer through. > > Ok, I see. I guess option 1 would be fine since we can't setup pointer > > width per address space in clang currently. However, spec doesn't provide > > any clarifications in this regard. > > > > So I guess using either `__global` or `__generic` for the pointer parameter > > would be fine... Or perhaps even leave it without any address space (i.e. > > _`_private`) and just addrspacecast from either `__global` or `__constant`. > > Do you have any preferences? > > > > As for `malloc` I am not sure that will work for OpenCL since we don't > > allow mem allocation on the device. Unless you mean the memory is allocated > > on a host... then I am not sure how it should work. > > Ok, I see. I guess option 1 would be fine since we can't setup pointer > > width per address space in clang currently. > > Really? What's missing there? It looks to me like `getPointerSize` does > take an address space. > > > So I guess using either __global or __generic for the pointer parameter > > would be fine... Or perhaps even leave it without any address space (i.e. > > _`_private`) and just addrspacecast from either __global or __constant. Do > > you have any preferences? > > `__private` is likely to be a smaller address space, right? I would > recommend using the fattest pointer that you want to actually support at > runtime — you shouldn't go all the way to `__generic` if the target relies on > eliminating that statically. If you want a target hook for the address space > of the notional `__cxa_atexit_context_pointer` typedef, I think that would be > reasonable. > > > As for malloc I am not sure that will work for OpenCL since we don't allow > > mem allocation on the device. Unless you mean the memory is allocated on a > > host... then I am not sure how it should work. > > Well, maybe not actually heap-allocated. I just think you should design the > ABI so that it's reasonably future-proof against taking any specific sort of > reasonable pointer. This cast only works if the address space is a subspace of the `__cxa_atexit` address space, right? Should we be checking that and emitting a diagnostic if that's not true? I think an IRGen-level diagnostic is fine here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
Anastasia marked an inline comment as done. Anastasia added inline comments. Comment at: lib/CodeGen/TargetInfo.h:274 +return LangAS::Default; + } + rjmccall wrote: > This target hook should just return the address space of the `__cxa_atexit` > argument, not to claim anything specific about the relation between that > address space and others. That is a global and permanent property of the > target ABI. We should *advise* targets to use an address space that other > major address spaces can be bitcast to, but ultimately that's just advisory. > > This would be a good thing to include in a PR to the Itanium C++ ABI, but it > would be reasonable to bundle all the address-space-related changes you need > in one PR. > This would be a good thing to include in a PR to the Itanium C++ ABI, but it > would be reasonable to bundle all the address-space-related changes you need > in one PR. I can try to do it but I would need some explanation of the process. :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
Anastasia updated this revision to Diff 209012.
Anastasia added a comment.
Changed API for target hook.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62413/new/
https://reviews.llvm.org/D62413
Files:
lib/CodeGen/CGDeclCXX.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/ItaniumCXXABI.cpp
lib/CodeGen/TargetInfo.h
test/CodeGenOpenCLCXX/atexit.cl
Index: test/CodeGenOpenCLCXX/atexit.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/atexit.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_cc1 %s -triple spir -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct S {
+ ~S(){};
+};
+S s;
+
+//CHECK-LABEL: define internal void @__cxx_global_var_init()
+//CHECK: call i32 @__cxa_atexit(void (i8*)* bitcast (void (%struct.S addrspace(4)*)* @_ZNU3AS41SD1Ev to void (i8*)*), i8* addrspacecast (i8 addrspace(1)* getelementptr inbounds (%struct.S, %struct.S addrspace(1)* @s, i32 0, i32 0) to i8*), i8 addrspace(1)* @__dso_handle)
+
+//CHECK: declare i32 @__cxa_atexit(void (i8*)*, i8*, i8 addrspace(1)*)
Index: lib/CodeGen/TargetInfo.h
===
--- lib/CodeGen/TargetInfo.h
+++ lib/CodeGen/TargetInfo.h
@@ -267,6 +267,11 @@
LangAS SrcAddr, LangAS DestAddr,
llvm::Type *DestTy) const;
+ /// Get address space of pointer parameter for __cxa_atexit.
+ virtual LangAS getAddrSpaceOfCxaAtexitPtrParam() const {
+return LangAS::Default;
+ }
+
/// Get the syncscope used in LLVM IR.
virtual llvm::SyncScope::ID getLLVMSyncScopeID(const LangOptions &LangOpts,
SyncScope Scope,
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -2284,8 +2284,19 @@
llvm::Type *dtorTy =
llvm::FunctionType::get(CGF.VoidTy, CGF.Int8PtrTy, false)->getPointerTo();
+ // Preserve address space of addr.
+ auto AddrAS = addr ? addr->getType()->getPointerAddressSpace() : 0;
+ auto AddrInt8PtrTy =
+ AddrAS ? CGF.Int8Ty->getPointerTo(AddrAS) : CGF.Int8PtrTy;
+
+ // Create a variable that binds the atexit to this shared object.
+ llvm::Constant *handle =
+ CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
+ auto *GV = cast(handle->stripPointerCasts());
+ GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
// extern "C" int __cxa_atexit(void (*f)(void *), void *p, void *d);
- llvm::Type *paramTys[] = { dtorTy, CGF.Int8PtrTy, CGF.Int8PtrTy };
+ llvm::Type *paramTys[] = {dtorTy, AddrInt8PtrTy, handle->getType()};
llvm::FunctionType *atexitTy =
llvm::FunctionType::get(CGF.IntTy, paramTys, false);
@@ -2294,12 +2305,6 @@
if (llvm::Function *fn = dyn_cast(atexit.getCallee()))
fn->setDoesNotThrow();
- // Create a variable that binds the atexit to this shared object.
- llvm::Constant *handle =
- CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
- auto *GV = cast(handle->stripPointerCasts());
- GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
-
if (!addr)
// addr is null when we are trying to register a dtor annotated with
// __attribute__((destructor)) in a constructor function. Using null here is
@@ -2309,7 +2314,7 @@
llvm::Value *args[] = {llvm::ConstantExpr::getBitCast(
cast(dtor.getCallee()), dtorTy),
- llvm::ConstantExpr::getBitCast(addr, CGF.Int8PtrTy),
+ llvm::ConstantExpr::getBitCast(addr, AddrInt8PtrTy),
handle};
CGF.EmitNounwindRuntimeCall(atexit, args);
}
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -3577,8 +3577,12 @@
llvm::Constant *
CodeGenModule::CreateRuntimeVariable(llvm::Type *Ty,
StringRef Name) {
- auto *Ret =
- GetOrCreateLLVMGlobal(Name, llvm::PointerType::getUnqual(Ty), nullptr);
+ auto PtrTy =
+ getContext().getLangOpts().OpenCL
+ ? llvm::PointerType::get(
+Ty, getContext().getTargetAddressSpace(LangAS::opencl_global))
+ : llvm::PointerType::getUnqual(Ty);
+ auto *Ret = GetOrCreateLLVMGlobal(Name, PtrTy, nullptr);
setDSOLocal(cast(Ret->stripPointerCasts()));
return Ret;
}
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -14,6 +14,7 @@
#include "CGCXXABI.h"
#include "CGObjCRuntime.h"
#include "CGOpenMPRuntime.h"
+#include "TargetInfo.h"
#include "clang/Basic/CodeGenOptions.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/Intrinsics.h"
@@ -118,9 +119,21 @@
CXXDestructorDecl *Dtor = Record->
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.h:274 +return LangAS::Default; + } + This target hook should just return the address space of the `__cxa_atexit` argument, not to claim anything specific about the relation between that address space and others. That is a global and permanent property of the target ABI. We should *advise* targets to use an address space that other major address spaces can be bitcast to, but ultimately that's just advisory. This would be a good thing to include in a PR to the Itanium C++ ABI, but it would be reasonable to bundle all the address-space-related changes you need in one PR. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
Anastasia updated this revision to Diff 207574.
Anastasia added a comment.
Added a target hook to query address spaces with the largest pointer width.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62413/new/
https://reviews.llvm.org/D62413
Files:
lib/CodeGen/CGDeclCXX.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/ItaniumCXXABI.cpp
lib/CodeGen/TargetInfo.h
test/CodeGenOpenCLCXX/atexit.cl
Index: test/CodeGenOpenCLCXX/atexit.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/atexit.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_cc1 %s -triple spir -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct S {
+ ~S(){};
+};
+S s;
+
+//CHECK-LABEL: define internal void @__cxx_global_var_init()
+//CHECK: call i32 @__cxa_atexit(void (i8*)* bitcast (void (%struct.S addrspace(4)*)* @_ZNU3AS41SD1Ev to void (i8*)*), i8* addrspacecast (i8 addrspace(1)* getelementptr inbounds (%struct.S, %struct.S addrspace(1)* @s, i32 0, i32 0) to i8*), i8 addrspace(1)* @__dso_handle)
+
+//CHECK: declare i32 @__cxa_atexit(void (i8*)*, i8*, i8 addrspace(1)*)
Index: lib/CodeGen/TargetInfo.h
===
--- lib/CodeGen/TargetInfo.h
+++ lib/CodeGen/TargetInfo.h
@@ -267,6 +267,12 @@
LangAS SrcAddr, LangAS DestAddr,
llvm::Type *DestTy) const;
+ /// Get address space with the largest size (pointer size in this address
+ /// space has the largest width for the target).
+ virtual LangAS getAddrSpaceWithLargestPtrSize() const {
+return LangAS::Default;
+ }
+
/// Get the syncscope used in LLVM IR.
virtual llvm::SyncScope::ID getLLVMSyncScopeID(const LangOptions &LangOpts,
SyncScope Scope,
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -2284,8 +2284,19 @@
llvm::Type *dtorTy =
llvm::FunctionType::get(CGF.VoidTy, CGF.Int8PtrTy, false)->getPointerTo();
+ // Preserve address space of addr.
+ auto AddrAS = addr ? addr->getType()->getPointerAddressSpace() : 0;
+ auto AddrInt8PtrTy =
+ AddrAS ? CGF.Int8Ty->getPointerTo(AddrAS) : CGF.Int8PtrTy;
+
+ // Create a variable that binds the atexit to this shared object.
+ llvm::Constant *handle =
+ CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
+ auto *GV = cast(handle->stripPointerCasts());
+ GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
// extern "C" int __cxa_atexit(void (*f)(void *), void *p, void *d);
- llvm::Type *paramTys[] = { dtorTy, CGF.Int8PtrTy, CGF.Int8PtrTy };
+ llvm::Type *paramTys[] = {dtorTy, AddrInt8PtrTy, handle->getType()};
llvm::FunctionType *atexitTy =
llvm::FunctionType::get(CGF.IntTy, paramTys, false);
@@ -2294,12 +2305,6 @@
if (llvm::Function *fn = dyn_cast(atexit.getCallee()))
fn->setDoesNotThrow();
- // Create a variable that binds the atexit to this shared object.
- llvm::Constant *handle =
- CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
- auto *GV = cast(handle->stripPointerCasts());
- GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
-
if (!addr)
// addr is null when we are trying to register a dtor annotated with
// __attribute__((destructor)) in a constructor function. Using null here is
@@ -2309,7 +2314,7 @@
llvm::Value *args[] = {llvm::ConstantExpr::getBitCast(
cast(dtor.getCallee()), dtorTy),
- llvm::ConstantExpr::getBitCast(addr, CGF.Int8PtrTy),
+ llvm::ConstantExpr::getBitCast(addr, AddrInt8PtrTy),
handle};
CGF.EmitNounwindRuntimeCall(atexit, args);
}
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -3577,8 +3577,12 @@
llvm::Constant *
CodeGenModule::CreateRuntimeVariable(llvm::Type *Ty,
StringRef Name) {
- auto *Ret =
- GetOrCreateLLVMGlobal(Name, llvm::PointerType::getUnqual(Ty), nullptr);
+ auto PtrTy =
+ getContext().getLangOpts().OpenCL
+ ? llvm::PointerType::get(
+Ty, getContext().getTargetAddressSpace(LangAS::opencl_global))
+ : llvm::PointerType::getUnqual(Ty);
+ auto *Ret = GetOrCreateLLVMGlobal(Name, PtrTy, nullptr);
setDSOLocal(cast(Ret->stripPointerCasts()));
return Ret;
}
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -14,6 +14,7 @@
#include "CGCXXABI.h"
#include "CGObjCRuntime.h"
#include "CGOpenMPRuntime.h"
+#include "TargetInfo.h"
#include "clang/Basic/CodeGenOptions.h"
#include "llvm/AD
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); Anastasia wrote: > rjmccall wrote: > > Anastasia wrote: > > > rjmccall wrote: > > > > Should this code be conditional to OpenCL? And why does `_cxa_atexit` > > > > take a `__global` pointer instead of, say, a `__generic` one? > > > The only objects that are destructible globally in OpenCL are `__global` > > > and `__constant`. However `__constant` isn't convertible to `__generic`. > > > Therefore, I am adding `__global` directly to avoid extra conversion. I > > > am not yet sure how to handle `__constant`though and how much destructing > > > objects in read-only memory segments would make sense anyway. I think I > > > will address this separately. > > The pointer argument to `__cxa_atexit` is just an arbitrary bit of context > > and doesn't have to actually be the address of a global. It's *convenient* > > to use the address of a global sometimes; e.g. you can use the global as > > the pointer and its destructor as the function, and then `__cxa_atexit` > > will just call the destructor for you without any additional code. But as > > far as the runtime is concerned, the pointer could be `malloc`'ed or > > something; we've never had a need to do that in the ABI, but it's good > > future-proofing to allow it. > > > > So there are three ways to get a global destructor to destroy a variable in > > `__constant`: > > - You can pass the pointer bitcast'ed as long as `sizeof(__constant void*) > > <= sizeof(__cxa_atexit_context_pointer)`. > > - You can ignore the argument and just materialize the address separately > > within the destructor function. > > - You can allocate memory for a context and then store the pointer in that. > > > > Obviously you should go with the one of the first two, but you should make > > sure your ABI doesn't preclude doing the latter in case it's useful for > > some future language feature. In other words, it doesn't really matter > > whether this argument is notionally in `__global` as long as that's wide > > enough to pass a more-or-less arbitrary pointer through. > Ok, I see. I guess option 1 would be fine since we can't setup pointer width > per address space in clang currently. However, spec doesn't provide any > clarifications in this regard. > > So I guess using either `__global` or `__generic` for the pointer parameter > would be fine... Or perhaps even leave it without any address space (i.e. > _`_private`) and just addrspacecast from either `__global` or `__constant`. > Do you have any preferences? > > As for `malloc` I am not sure that will work for OpenCL since we don't allow > mem allocation on the device. Unless you mean the memory is allocated on a > host... then I am not sure how it should work. > Ok, I see. I guess option 1 would be fine since we can't setup pointer width > per address space in clang currently. Really? What's missing there? It looks to me like `getPointerSize` does take an address space. > So I guess using either __global or __generic for the pointer parameter would > be fine... Or perhaps even leave it without any address space (i.e. > _`_private`) and just addrspacecast from either __global or __constant. Do > you have any preferences? `__private` is likely to be a smaller address space, right? I would recommend using the fattest pointer that you want to actually support at runtime — you shouldn't go all the way to `__generic` if the target relies on eliminating that statically. If you want a target hook for the address space of the notional `__cxa_atexit_context_pointer` typedef, I think that would be reasonable. > As for malloc I am not sure that will work for OpenCL since we don't allow > mem allocation on the device. Unless you mean the memory is allocated on a > host... then I am not sure how it should work. Well, maybe not actually heap-allocated. I just think you should design the ABI so that it's reasonably future-proof against taking any specific sort of reasonable pointer. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
Anastasia marked an inline comment as done. Anastasia added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); rjmccall wrote: > Anastasia wrote: > > rjmccall wrote: > > > Should this code be conditional to OpenCL? And why does `_cxa_atexit` > > > take a `__global` pointer instead of, say, a `__generic` one? > > The only objects that are destructible globally in OpenCL are `__global` > > and `__constant`. However `__constant` isn't convertible to `__generic`. > > Therefore, I am adding `__global` directly to avoid extra conversion. I am > > not yet sure how to handle `__constant`though and how much destructing > > objects in read-only memory segments would make sense anyway. I think I > > will address this separately. > The pointer argument to `__cxa_atexit` is just an arbitrary bit of context > and doesn't have to actually be the address of a global. It's *convenient* > to use the address of a global sometimes; e.g. you can use the global as the > pointer and its destructor as the function, and then `__cxa_atexit` will just > call the destructor for you without any additional code. But as far as the > runtime is concerned, the pointer could be `malloc`'ed or something; we've > never had a need to do that in the ABI, but it's good future-proofing to > allow it. > > So there are three ways to get a global destructor to destroy a variable in > `__constant`: > - You can pass the pointer bitcast'ed as long as `sizeof(__constant void*) <= > sizeof(__cxa_atexit_context_pointer)`. > - You can ignore the argument and just materialize the address separately > within the destructor function. > - You can allocate memory for a context and then store the pointer in that. > > Obviously you should go with the one of the first two, but you should make > sure your ABI doesn't preclude doing the latter in case it's useful for some > future language feature. In other words, it doesn't really matter whether > this argument is notionally in `__global` as long as that's wide enough to > pass a more-or-less arbitrary pointer through. Ok, I see. I guess option 1 would be fine since we can't setup pointer width per address space in clang currently. However, spec doesn't provide any clarifications in this regard. So I guess using either `__global` or `__generic` for the pointer parameter would be fine... Or perhaps even leave it without any address space (i.e. _`_private`) and just addrspacecast from either `__global` or `__constant`. Do you have any preferences? As for `malloc` I am not sure that will work for OpenCL since we don't allow mem allocation on the device. Unless you mean the memory is allocated on a host... then I am not sure how it should work. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); Anastasia wrote: > rjmccall wrote: > > Should this code be conditional to OpenCL? And why does `_cxa_atexit` take > > a `__global` pointer instead of, say, a `__generic` one? > The only objects that are destructible globally in OpenCL are `__global` and > `__constant`. However `__constant` isn't convertible to `__generic`. > Therefore, I am adding `__global` directly to avoid extra conversion. I am > not yet sure how to handle `__constant`though and how much destructing > objects in read-only memory segments would make sense anyway. I think I will > address this separately. The pointer argument to `__cxa_atexit` is just an arbitrary bit of context and doesn't have to actually be the address of a global. It's *convenient* to use the address of a global sometimes; e.g. you can use the global as the pointer and its destructor as the function, and then `__cxa_atexit` will just call the destructor for you without any additional code. But as far as the runtime is concerned, the pointer could be `malloc`'ed or something; we've never had a need to do that in the ABI, but it's good future-proofing to allow it. So there are three ways to get a global destructor to destroy a variable in `__constant`: - You can pass the pointer bitcast'ed as long as `sizeof(__constant void*) <= sizeof(__cxa_atexit_context_pointer)`. - You can ignore the argument and just materialize the address separately within the destructor function. - You can allocate memory for a context and then store the pointer in that. Obviously you should go with the one of the first two, but you should make sure your ABI doesn't preclude doing the latter in case it's useful for some future language feature. In other words, it doesn't really matter whether this argument is notionally in `__global` as long as that's wide enough to pass a more-or-less arbitrary pointer through. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
Anastasia marked an inline comment as done. Anastasia added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); rjmccall wrote: > Should this code be conditional to OpenCL? And why does `_cxa_atexit` take a > `__global` pointer instead of, say, a `__generic` one? The only objects that are destructible globally in OpenCL are `__global` and `__constant`. However `__constant` isn't convertible to `__generic`. Therefore, I am adding `__global` directly to avoid extra conversion. I am not yet sure how to handle `__constant`though and how much destructing objects in read-only memory segments would make sense anyway. I think I will address this separately. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
Anastasia updated this revision to Diff 202708.
Anastasia added a comment.
- Guard OpenCL specific IR generation under OpenCL lang mode
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62413/new/
https://reviews.llvm.org/D62413
Files:
lib/CodeGen/CGDeclCXX.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/ItaniumCXXABI.cpp
test/CodeGenOpenCLCXX/atexit.cl
Index: test/CodeGenOpenCLCXX/atexit.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/atexit.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_cc1 %s -triple spir -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct S {
+ ~S(){};
+};
+S s;
+
+//CHECK-LABEL: define internal void @__cxx_global_var_init()
+// call i32 @__cxa_atexit(void (i8*)* bitcast (void (%struct.S addrspace(4)*)* @_ZNU3AS41SD1Ev to void (i8*)*), i8 addrspace(1)* getelementptr inbounds (%struct.S, %struct.S addrspace(1)* @s, i32 0, i32 0), i8 addrspace(1)* @__dso_handle)
+
+//declare i32 @__cxa_atexit(void (i8*)*, i8 addrspace(1)*, i8 addrspace(1)*)
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -2282,8 +2282,19 @@
llvm::Type *dtorTy =
llvm::FunctionType::get(CGF.VoidTy, CGF.Int8PtrTy, false)->getPointerTo();
+ // Preserve address space of addr.
+ auto AddrAS = addr ? addr->getType()->getPointerAddressSpace() : 0;
+ auto AddrInt8PtrTy =
+ AddrAS ? CGF.Int8Ty->getPointerTo(AddrAS) : CGF.Int8PtrTy;
+
+ // Create a variable that binds the atexit to this shared object.
+ llvm::Constant *handle =
+ CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
+ auto *GV = cast(handle->stripPointerCasts());
+ GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
// extern "C" int __cxa_atexit(void (*f)(void *), void *p, void *d);
- llvm::Type *paramTys[] = { dtorTy, CGF.Int8PtrTy, CGF.Int8PtrTy };
+ llvm::Type *paramTys[] = {dtorTy, AddrInt8PtrTy, handle->getType()};
llvm::FunctionType *atexitTy =
llvm::FunctionType::get(CGF.IntTy, paramTys, false);
@@ -2292,12 +2303,6 @@
if (llvm::Function *fn = dyn_cast(atexit.getCallee()))
fn->setDoesNotThrow();
- // Create a variable that binds the atexit to this shared object.
- llvm::Constant *handle =
- CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
- auto *GV = cast(handle->stripPointerCasts());
- GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
-
if (!addr)
// addr is null when we are trying to register a dtor annotated with
// __attribute__((destructor)) in a constructor function. Using null here is
@@ -2307,7 +2312,7 @@
llvm::Value *args[] = {llvm::ConstantExpr::getBitCast(
cast(dtor.getCallee()), dtorTy),
- llvm::ConstantExpr::getBitCast(addr, CGF.Int8PtrTy),
+ llvm::ConstantExpr::getBitCast(addr, AddrInt8PtrTy),
handle};
CGF.EmitNounwindRuntimeCall(atexit, args);
}
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -3566,8 +3566,12 @@
llvm::Constant *
CodeGenModule::CreateRuntimeVariable(llvm::Type *Ty,
StringRef Name) {
- auto *Ret =
- GetOrCreateLLVMGlobal(Name, llvm::PointerType::getUnqual(Ty), nullptr);
+ auto PtrTy =
+ getContext().getLangOpts().OpenCL
+ ? llvm::PointerType::get(
+Ty, getContext().getTargetAddressSpace(LangAS::opencl_global))
+ : llvm::PointerType::getUnqual(Ty);
+ auto *Ret = GetOrCreateLLVMGlobal(Name, PtrTy, nullptr);
setDSOLocal(cast(Ret->stripPointerCasts()));
return Ret;
}
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -14,6 +14,7 @@
#include "CGCXXABI.h"
#include "CGObjCRuntime.h"
#include "CGOpenMPRuntime.h"
+#include "TargetInfo.h"
#include "clang/Basic/CodeGenOptions.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/Intrinsics.h"
@@ -118,9 +119,22 @@
CXXDestructorDecl *Dtor = Record->getDestructor();
Func = CGM.getAddrAndTypeOfCXXStructor(GlobalDecl(Dtor, Dtor_Complete));
-Argument = llvm::ConstantExpr::getBitCast(
-Addr.getPointer(), CGF.getTypes().ConvertType(Type)->getPointerTo());
-
+if (CGF.getContext().getLangOpts().OpenCL) {
+ // FIXME: A solution is needed for opencl_constant. We could create
+ // atexit_constant, but more generic solution would probably be to mangle
+ // address space?
+ auto DestTy = CGF.getTypes().ConvertType(Type)->getPointerTo(
+ CGM.getContext().getTargetAddressSpace(LangAS::opencl_global));
+ auto SrcAS = D.getType().getQualifiers().getAddressSpace();
+ if (LangAS::opencl_global == Src
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); Should this code be conditional to OpenCL? And why does `_cxa_atexit` take a `__global` pointer instead of, say, a `__generic` one? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors
Anastasia created this revision.
Anastasia added a reviewer: rjmccall.
Herald added subscribers: ebevhan, yaxunl.
Preserve address spaces of global objects while generating 'atexit' stub.
https://reviews.llvm.org/D62413
Files:
lib/CodeGen/CGDeclCXX.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/ItaniumCXXABI.cpp
test/CodeGenOpenCLCXX/atexit.cl
Index: test/CodeGenOpenCLCXX/atexit.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/atexit.cl
@@ -0,0 +1,11 @@
+//RUN: %clang_cc1 %s -triple spir -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct S {
+ ~S(){};
+};
+S s;
+
+//CHECK-LABEL: define internal void @__cxx_global_var_init()
+// call i32 @__cxa_atexit(void (i8*)* bitcast (void (%struct.S addrspace(4)*)* @_ZNU3AS41SD1Ev to void (i8*)*), i8 addrspace(1)* getelementptr inbounds (%struct.S, %struct.S addrspace(1)* @s, i32 0, i32 0), i8 addrspace(1)* @__dso_handle)
+
+//declare i32 @__cxa_atexit(void (i8*)*, i8 addrspace(1)*, i8 addrspace(1)*)
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -2299,8 +2299,19 @@
llvm::Type *dtorTy =
llvm::FunctionType::get(CGF.VoidTy, CGF.Int8PtrTy, false)->getPointerTo();
+ // Preserve address space of addr.
+ auto AddrAS = addr ? addr->getType()->getPointerAddressSpace() : 0;
+ auto AddrInt8PtrTy =
+ AddrAS ? CGF.Int8Ty->getPointerTo(AddrAS) : CGF.Int8PtrTy;
+
+ // Create a variable that binds the atexit to this shared object.
+ llvm::Constant *handle =
+ CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
+ auto *GV = cast(handle->stripPointerCasts());
+ GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
// extern "C" int __cxa_atexit(void (*f)(void *), void *p, void *d);
- llvm::Type *paramTys[] = { dtorTy, CGF.Int8PtrTy, CGF.Int8PtrTy };
+ llvm::Type *paramTys[] = {dtorTy, AddrInt8PtrTy, handle->getType()};
llvm::FunctionType *atexitTy =
llvm::FunctionType::get(CGF.IntTy, paramTys, false);
@@ -2309,12 +2320,6 @@
if (llvm::Function *fn = dyn_cast(atexit.getCallee()))
fn->setDoesNotThrow();
- // Create a variable that binds the atexit to this shared object.
- llvm::Constant *handle =
- CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
- auto *GV = cast(handle->stripPointerCasts());
- GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
-
if (!addr)
// addr is null when we are trying to register a dtor annotated with
// __attribute__((destructor)) in a constructor function. Using null here is
@@ -2324,7 +2329,7 @@
llvm::Value *args[] = {llvm::ConstantExpr::getBitCast(
cast(dtor.getCallee()), dtorTy),
- llvm::ConstantExpr::getBitCast(addr, CGF.Int8PtrTy),
+ llvm::ConstantExpr::getBitCast(addr, AddrInt8PtrTy),
handle};
CGF.EmitNounwindRuntimeCall(atexit, args);
}
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -3533,8 +3533,12 @@
llvm::Constant *
CodeGenModule::CreateRuntimeVariable(llvm::Type *Ty,
StringRef Name) {
- auto *Ret =
- GetOrCreateLLVMGlobal(Name, llvm::PointerType::getUnqual(Ty), nullptr);
+ auto PtrTy =
+ getContext().getLangOpts().OpenCL
+ ? llvm::PointerType::get(
+Ty, getContext().getTargetAddressSpace(LangAS::opencl_global))
+ : llvm::PointerType::getUnqual(Ty);
+ auto *Ret = GetOrCreateLLVMGlobal(Name, PtrTy, nullptr);
setDSOLocal(cast(Ret->stripPointerCasts()));
return Ret;
}
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -14,6 +14,7 @@
#include "CGCXXABI.h"
#include "CGObjCRuntime.h"
#include "CGOpenMPRuntime.h"
+#include "TargetInfo.h"
#include "clang/Basic/CodeGenOptions.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/IR/Intrinsics.h"
@@ -118,8 +119,17 @@
CXXDestructorDecl *Dtor = Record->getDestructor();
Func = CGM.getAddrAndTypeOfCXXStructor(GlobalDecl(Dtor, Dtor_Complete));
-Argument = llvm::ConstantExpr::getBitCast(
-Addr.getPointer(), CGF.getTypes().ConvertType(Type)->getPointerTo());
+// FIXME: A solution is needed for opencl_constant. We could create
+// atexit_constant, but more generic solution would probably be to mangle
+// address space?
+auto DestTy = CGF.getTypes().ConvertType(Type)->getPointerTo(
+CGM.getContext().getTargetAddressSpace(LangAS::opencl_global));
+auto SrcAS = D.getType().getQualifiers().getAddressSpace();
+if (LangAS::opencl_global == SrcAS)
+ Argument = llvm::ConstantExpr::getBitCast(Addr.getPointer(), DestTy);
