[PATCH] D58346: [Sema] Change addr space diagnostics in casts to follow C++ style

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia marked an inline comment as done.
Anastasia added inline comments.


================
Comment at: lib/Sema/SemaCast.cpp:2224
+  } else if (IsLValueCast) {
     Kind = CK_LValueBitCast;
   } else if (DestType->isObjCObjectPointerType()) {
----------------
Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > ebevhan wrote:
> > > > This might not be applicable to this patch, but just something I noticed.
> > > > 
> > > > So `reinterpret_cast` only operates on pointers when dealing with address spaces. What about something like
> > > > ```
> > > > T a;
> > > > T& a_ref = reinterpret_cast<T&>(a);
> > > > ```
> > > > `reinterpret_cast` on an lvalue like this is equivalent to `*reinterpret_cast<T*>(&a)`. So for AS code:
> > > > ```
> > > > __private T x;
> > > > __generic T& ref = reinterpret_cast<__generic T&>(x);
> > > > ```
> > > > This should work, since `*reinterpret_cast<__generic T*>(&x)` is valid, correct?
> > > > 
> > > > What if we have the reference cast case with a different address space like this? Doesn't the `IsLValueCast` check need to be first?
> > > > 
> > > > What if we have the reference cast case with a different address space like this? Doesn't the IsLValueCast check need to be first?
> > > 
> > > Ok, let me see if I understand you. I changed `__generic` -> `__global` since it's invalid and the error is produced as follows:
> > >   test.cl:7:21: error: reinterpret_cast from 'int' to '__global int &' is not allowed
> > >   __global int& ref = reinterpret_cast<__global int&>(x);
> > > 
> > > Is this not what we are expecting here?
> > My last sentence could have been a bit clearer. Yes, for the `__private`->`__global` case, it should error. But I was thinking more of the case where the AS conversion is valid, like `__private`->`__generic`. Then we will do the AS conversion, but we should have done both an AS conversion and an `LValueBitCast`, because we need to both convert the 'pointer' and also dereference it.
> Hmm... it seems like here we can only have one cast kind... I guess `CK_LValueBitCast` leads to the generation of `bitcast` in the IR? But `addrspacecast` can perform both bit reinterpretation and address translation, so perhaps it makes sense to only have an address space conversion in this case? Unless some other logic is needed elsewhere before CodeGen... I will try to construct a test case in plain C++. 
I am not sure I understood the problem you are describing, may be I need an example...

But the valid address space conversion example in OpenCL

```
__private T x = ...;
__generic T& ref = reinterpret_cast<__generic T&>(x);
```
produces correctly `addrspacecast`

```
  %0 = load i32*, i32** %x.addr, align 4
  %1 = addrspacecast i32* %0 to i32 addrspace(4)*
  store i32 addrspace(4)* %1, i32 addrspace(4)** %ref
```

However, if we keep checking `CK_LValueBitCast` first clang attempts to generate `bitcast` but fails because pointers are in different address spaces.


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

https://reviews.llvm.org/D58346