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
