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
