[OpenCL] Fix address space for implicit conversion (PR43145)

[svenvh]

Summary

Clang was creating a DerivedToBase ImplicitCastExpr that was also
casting between address spaces as part of the second step in the
standard conversion sequence. Defer the address space conversion to
the third step in the sequence instead, such that we get a separate
ImplicitCastExpr for the address space conversion.

[rjmccall]

Is there a similar problem with reference parameters?

[svenvh]

In D70605#1759614, @rjmccall wrote:

Is there a similar problem with reference parameters?

It doesn't seem so, patching the test as follows gives reasonable output.

@@ -73,8 +73,10 @@ void pr43145_3(int n) {
 // Implicit conversion of derived to base.
 
 void functionWithBaseArg(class B2 *b) {}
+void functionWithBaseArgRef(class B2 &b) {}
 
 void pr43145_4() {
   Derived d;
   functionWithBaseArg(&d);
+  functionWithBaseArgRef(d);
 }

[rjmccall]

Great. Could you add that to the test, then, just to make sure we don't regress it?

[svenvh]

Added test for references too.

[rjmccall]

clang/lib/Sema/SemaExprCXX.cpp

4108

Hmm, sorry, just noticed this. Could you write this so that it preserves the original kind of pointer of ToType? I think it has to be a PointerType, BlockPointerType, or ObjCObjectPointerType. It doesn't look like there's an existing utility for that.

[svenvh]

Address comment from @rjmccall to preserve original ToType pointer type.

[rjmccall]

Thanks, LGTM.

[ebevhan]

I know this is some really late feedback on this patch. I struck upon some issues with while rebasing D62574.

clang/lib/Sema/SemaExprCXX.cpp

4106

I don't think this will manage to properly repack the type if it's hidden behind enough sugar. When I enable C++ address space conversions in a non-OpenCL context, this breaks the derived-to-base example in CodeGenCXX/address-space-cast.cpp.

IsPointerConversion doesn't have an issue with this since it reconstructs the destination type with the appropriate qualifiers through BuildSimilarlyQualifiedPointerType.

Wouldn't it make more sense to have *PointerConversion only handle the derived-to-base and leave the address space conversion to *QualificationConversion?

[svenvh]

clang/lib/Sema/SemaExprCXX.cpp

4106

Wouldn't it make more sense to have *PointerConversion only handle the derived-to-base and leave the address space conversion to *QualificationConversion?

I didn't have a thorough look to understand all implications of that, but maybe it's worth trying. Not sure I'll be able to follow up on this any time soon, would you be able to give it a try?

[ebevhan]

clang/lib/Sema/SemaExprCXX.cpp

4106

I'll see if I can have a look, or at least make the existing implementation a bit more thorough.

To be entirely honest, I feel a bit stupid right now: I forgot that I added the derived-to-base test in the test case I mentioned in my local experimental branch, so of course you wouldn't be able to use that as a baseline. Oops!

clang/lib/Sema/SemaExprCXX.cpp

if (SCS.IncompatibleObjC && Action != AA_Casting) {
       if (Action == AA_Initializing)
         Diag(From->getBeginLoc(), diag::err_arc_weak_unavailable_assign);
       else
         Diag(From->getBeginLoc(), diag::err_arc_convesion_of_weak_unavailable)
             << (Action == AA_Casting) << From->getType() << ToType
             << From->getSourceRange();
     }
 
+    // Defer address space conversion to the third conversion.
+    QualType FromPteeType = From->getType()->getPointeeType();
+    QualType ToPteeType = ToType->getPointeeType();
+    QualType NewToType = ToType;
+    if (!FromPteeType.isNull() && !ToPteeType.isNull() &&
+        FromPteeType.getAddressSpace() != ToPteeType.getAddressSpace()) {
+      NewToType = Context.removeAddrSpaceQualType(ToPteeType);
+      NewToType = Context.getAddrSpaceQualType(NewToType,
+                                               FromPteeType.getAddressSpace());
+      if (ToType->isObjCObjectPointerType())
+        NewToType = Context.getObjCObjectPointerType(NewToType);
+      else if (ToType->isBlockPointerType())
+        NewToType = Context.getBlockPointerType(NewToType);
+      else
+        NewToType = Context.getPointerType(NewToType);
+    }
+
     CastKind Kind;
     CXXCastPath BasePath;
-    if (CheckPointerConversion(From, ToType, Kind, BasePath, CStyle))
+    if (CheckPointerConversion(From, NewToType, Kind, BasePath, CStyle))
       return ExprError();
 
     // Make sure we extend blocks if necessary.
     // FIXME: doing this here is really ugly.
     if (Kind == CK_BlockPointerToObjCPointerCast) {
       ExprResult E = From;
       (void) PrepareCastToObjCObjectPointer(E);
       From = E.get();
     }
     if (getLangOpts().allowsNonTrivialObjCLifetimeQualifiers())
-      CheckObjCConversion(SourceRange(), ToType, From, CCK);
-    From = ImpCastExprToType(From, ToType, Kind, VK_RValue, &BasePath, CCK)
+      CheckObjCConversion(SourceRange(), NewToType, From, CCK);
+    From = ImpCastExprToType(From, NewToType, Kind, VK_RValue, &BasePath, CCK)
              .get();
     break;
   }
 
   case ICK_Pointer_Member: {
     CastKind Kind;
     CXXCastPath BasePath;
     if (CheckMemberPointerConversion(From, ToType, Kind, BasePath, CStyle))

clang/test/CodeGenOpenCLCXX/addrspace-derived-base.cl

void pr43145_3(int n) {
 
   private Derived *pd = &d;
   pd->getRef() = n;
 
   // CHECK: addrspacecast %class.Derived* %4 to %class.Derived addrspace(4)*
   // CHECK: bitcast i8 addrspace(4)* %add.ptr1 to %class.B2 addrspace(4)*
   // CHECK: call {{.*}} @_ZNU3AS42B26getRefEv
 }
+
+// Implicit conversion of derived to base.
+
+void functionWithBaseArgPtr(class B2 *b) {}
+void functionWithBaseArgRef(class B2 &b) {}
+
+void pr43145_4() {
+  Derived d;
+  functionWithBaseArgPtr(&d);
+  functionWithBaseArgRef(d);
+}