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 it would fail to compile for `__constant`. > > > > > You can pass the pointer bitcast'ed as long as sizeof(__constant void*) <= > > sizeof(__cxa_atexit_context_pointer). > > Do you think I should leave a `bitcast` then? Not sure if something might > assert in LLVM though if there is a `bitcast` between pointers to different > address space... so I am confused... I think LLVM doesn't allow direct bitcasts between different address spaces (to help eliminate obvious bugs), but you can do it with `ptrtoint`. For generality, though, you should emit a diagnostic if the `__cxa_atexit` pointer size is actually smaller than the target pointer. Alternatively, I'm not sure we actually rely on this pointer for anything right now, so you might be able to just use null if the address spaces are different. That decision will eventually need to be able to affect how we generate the global destructor function, though. 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
