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

Wed Feb 20 06:26:57 PST 2019

ebevhan 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:
> > > 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.
> > > 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?
> > 
> > That's likely, yes. The dereference in the example disappears if we assign to a `T&` and it becomes implied if we assign to a `T` through lvalue-to-rvalue conversion, so in both cases we're taking the address of something and then converting the address to a different type.
> > 
> > > 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++.
> > 
> > Oh, you're right! For some reason I was thinking that `addrspacecast` could only change the addrspace of the argument, but apparently it could change the pointee type too. At least according to the langref. So long as nothing in Clang assumes that a CK_AddressSpaceConversion can't change the pointee type as well as the address space I guess it should be safe.
> > 
> > When I try a 'simultaneous' conversion in C, I don't get a single addrspacecast, though: https://godbolt.org/z/Q818yW So I wonder if existing code handles this.
> > 
> > A test case would be good.
> Sorry, I haven't seen your previous comment before sending mine. :)
> 
> 
> > 
> > When I try a 'simultaneous' conversion in C, I don't get a single addrspacecast
> 
> I think 2 conversions in IR are not necessary, not sure how we ended up doing this though... but we had related discussion on that while implementing some OpenCL features and the conclusion was that we should just have an `addrspacecast`. Do you think we should correct this in C?
> 
> 
> 
> > When I try a 'simultaneous' conversion in C, I don't get a single addrspacecast, though: https://godbolt.org/z/Q818yW So I wonder if existing code handles this.
> 
> Ok, I will check whether we cover this with any CodeGen test and if not I will add one.
> 
> 
> I think 2 conversions in IR are not necessary, not sure how we ended up doing this though... but we had related discussion on that while implementing some OpenCL features and the conclusion was that we should just have an addrspacecast. Do you think we should correct this in C?

Yes, I don't think it's necessary either, the addrspacecast should be enough.

Looking closer at the emission in the C case, Clang actually emits a single addrspacecast. Something else in LLVM is splitting it up into an addrspacecast and a bitcast. So it seems to be working just fine.

I was just concerned that we might hit edge cases where one conversion wasn't enough to convert both the address space and the pointee type, but it seems like CK_AddressSpaceConversion acts as both if it needs to. So it all seems good!

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

https://reviews.llvm.org/D58346