[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

Mon May 22 10:18:37 PDT 2023

jhuber6 added a comment.

In D151087#4361367 <https://reviews.llvm.org/D151087#4361367>, @ebevhan wrote:

> I don't think the standard argument really holds. It doesn't mention restrictions on address spaces because it doesn't have to, since they don't exist in C++. If they did, I'm pretty sure that reinterpret_cast would disallow arbitrary address space-modifying casts, since they wouldn't necessarily be bitcasts.

The reason cited for this change was that the standard does not allow `reinterpret_cast` to drop qualifiers, I don't think that's true so we're free to define our own behavior. Whether or not this is safe or desirable is up to us, and I'm arguing that for C++ it should be permitted.

> Like @arsenm said, any behaviors that you're using or observing regarding conversion of target address spaces in both C and C++ are purely coincidental. I don't think it's a great idea to add more such coincidental behaviors. The result will be that future code will become dependent on these arbitrary, less restrictive behaviors, and it will be much harder to properly define sensible semantics later on.

The behavior isn't coincidental, it does exactly what you expect if you were to use the same address space in LLVM-IR. The problem is that there are no semantic checks on them so if you use them incorrectly it will break. I feel like this is a separate issue and I don't know why it would prevent us from doing *any* kind of address space cast in C++.  There is no alternative in C++ and we already permit this with C-style casts, we cannot use OpenCL extensions like `addrspace_cast` so that leaves us unable to use C++ to write programs we want to write. We already use numerical address spaces in the OpenMP GPU runtime, and internally there we need to use C to do all the address space casts because of this.

There would be definite value in using the target's information to map address spaces to the known OpenCL ones so we could share some of their checks, but I feel like that's a separate issue.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151087/new/

https://reviews.llvm.org/D151087