[PATCH] D58236: Make address space conversions a bit stricter.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6996
+  "|%diff{casting $ to type $|casting between types}0,1}2"
+  " changes address space of nested pointer">;
 def err_typecheck_incompatible_ownership : Error<
----------------
I am wondering if we could just unify with the diagnostic above?

We could add another select at the end:
  " changes address space of %select{nested|}3 pointer"


================
Comment at: lib/Sema/SemaCast.cpp:2294
+
+  // FIXME: C++ might want to emit different errors here.
   if (Self.getLangOpts().OpenCL) {
----------------
I think my patch is divorcing C++ from here https://reviews.llvm.org/D58346.

Although, I might need to update it to add the same checks. I can do it once your patch is finalized. :)


================
Comment at: lib/Sema/SemaCast.cpp:2295
+  // FIXME: C++ might want to emit different errors here.
   if (Self.getLangOpts().OpenCL) {
+    const Type *DestPtr, *SrcPtr;
----------------
ebevhan wrote:
> I'd also like to petition to remove the guard on OpenCL here. Maybe an RFC for formalizing the support for address space conversion semantics is in order?
Yes, I'd support that! It would be good to generalize the rules as much as we can rather than adding extra checks for languages (the semantic is very similar).

As a matter of fact I tried to do the same in https://reviews.llvm.org/D58346 in `TryAddressSpaceCast` and some C++ tests fail. So I had to add OpenCL check for now. :( Perhaps, they could just be changed but needs agreement with the community first.


================
Comment at: lib/Sema/SemaExpr.cpp:14199
+    // qualifiers.
+    // XXX: Canonical types?
+    const Type *SrcPtr = SrcType->getPointeeType().getTypePtr();
----------------
May be, since if we are using a typedef that is a pointer type `isPointerType()` might not return true? I would just extend a test case with typedef to a pointer type as one of the nested levels.


================
Comment at: test/CodeGenOpenCL/numbered-address-space.cl:17
-void test_numbered_as_to_builtin(__attribute__((address_space(42))) int *arbitary_numbered_ptr, float src) {
-  volatile float result = __builtin_amdgcn_ds_fmaxf(arbitary_numbered_ptr, src, 0, 0, false);
-}
----------------
Does this not compile any more?


================
Comment at: test/SemaOpenCL/address-spaces.cl:89
+  __local int * __global * __private * lll;
+  lll = gg; // expected-warning {{incompatible pointer types assigning to '__local int *__global **' from '__global int **'}}
+}
----------------
ebevhan wrote:
> This doesn't seem entirely correct still, but I'm not sure what to do about it.
Is it because `Sema::IncompatiblePointer` has priority? We might want to change that. I think it was ok before because qualifier's mismatch was only a warning but now with the address spaces we are giving an error. I wonder if adding a separate enum item for address spaces (something like `Sema::IncompatibleNestedPointerAddressSpace`) would simplify things.


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

https://reviews.llvm.org/D58236