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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to