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 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. Thanks! I will go for `NULL` for now to prevent ICE and unblock some further work. But I added a FIXME to make sure to come back to this. I would then either change the dtor generation of emit some meaningful non-NULL value as an argument. 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
