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

[Bevin Hansson via Phabricator via cfe-commits]

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:
> > > 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++. 
> 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.


================
Comment at: lib/Sema/SemaCast.cpp:2309
+    auto DestPointeeTypeWithoutAS = Self.Context.removeAddrSpaceQualType(
+        DestPointeeType.getCanonicalType());
+    return Self.Context.hasSameType(SrcPointeeTypeWithoutAS,
----------------
Anastasia wrote:
> Anastasia wrote:
> > ebevhan wrote:
> > > Maybe I'm mistaken, but won't getting the canonical type here drop qualifiers (like cv) in nested pointers? If so, an addrspace_cast might strip qualifiers by mistake.
> > Yes, indeed I will need to extend this to nested pointers when we are ready. But for now I can try to change this bit... however I am not sure it will work w/o canonical types when we have typedef. I will try to create an example and see.
> I checked the canonical type does preserve the qualifiers correctly.
> 
> Here is the AST dump of the following C type `mytype const __generic*` where  `typedef  __generic int* mytype;`.
> 
> 
> ```
> PointerType 0x204d3b0 'const __generic mytype *'
> `-QualType 0x204d369 'const __generic mytype' const __generic
>   `-TypedefType 0x204d320 'mytype' sugar
>     |-Typedef 0x204d1b0 'mytype'
>     `-PointerType 0x204d170 '__generic int *'
>       `-QualType 0x204d158 '__generic int' __generic
>         `-BuiltinType 0x2009750 'int'
> ```
> 
> and it's canonical representation in AST is:
> 
> ```
> PointerType 0x204d380 '__generic int *const __generic *'
> `-QualType 0x204d349 '__generic int *const __generic' const __generic
>   `-PointerType 0x204d170 '__generic int *'
>     `-QualType 0x204d158 '__generic int' __generic
>       `-BuiltinType 0x2009750 'int'
> ```
> 
> So using canonical type will just simply handling of nested pointer chain by avoiding special casing typedefs. We won't loose any qualifiers.
Okay, seems good then!


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

https://reviews.llvm.org/D58346