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_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... 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
