Sema: Fix explicit address space cast in C++

[yaxunl]

Summary

Currently clang does not allow implicit cast of a pointer to a pointer type in different address space but allows C-style cast of a pointer to a pointer type in different address space. However, there is a bug in Sema causing incorrect Cast Expr in AST for the latter case, which in turn results in invalid LLVM IR in codegen.

This is because Sema::IsQualificationConversion returns true for a cast of pointer to a pointer type in different address space, which in turn allows a standard conversion and results in a cast expression with no op in AST.

This patch fixes that by let Sema::IsQualificationConversion returns false for a cast of pointer to a pointer type in different address space, which in turn disallows standard conversion, implicit cast, and static cast. Finally it results in an reinterpret cast and correct conversion kind is set.

[rjmccall]

lib/Sema/SemaCast.cpp

2171 ↗

(On Diff #155353)

Please extract this to a separate function so you can reuse intermediate results.

And in the presence of address-space promotions, you need to do a promotion followed by a reinterpretation, right?

lib/Sema/SemaOverload.cpp

3150 ↗

(On Diff #155353)

It's not really OpenCL C++ that's special here, it's the possibility of promotions between address spaces.

[yaxunl]

lib/Sema/SemaOverload.cpp

3150 ↗

(On Diff #155353)

For OpenCL C++, there is language rule about address space promotion.

For other languages, there is no generic rule about adderss space promotion, since the meaning of address space and their relations are target dependent. Do we want to introduce a target hook Target::canPromote(AddressSpace Src, AddressSpace Dest, LanguageOptions LangOpts) to represent this? Or do we just assume a simple rule, that is, all address space can be promoted to default address space 0, otherwise it is not allowed?

[rjmccall]

lib/Sema/SemaOverload.cpp

3150 ↗

(On Diff #155353)

A target-specific hook for handling target-specific address spaces makes sense to me. I don't think there's a generic rule allowing promotion into the default AS.

[yaxunl]

lib/Sema/SemaOverload.cpp

3150 ↗

(On Diff #155353)

I checked the current logic about address space promotions. There are several functions Type::Qualifier::isAddressSpaceSupersetOf, Type:;Qualifier::compatiblyIncludes, PointerType::isAddressSpaceOverlapping which checks address space compatibility. However they are only based on language rules and do not depend on targets. Since only OpenCL defined language rules regarding address spaces, address space promotion is only allowed for OpenCL.

To allow target specific address space promotion, these functions need an extra ASTContext parameter to allow them access LanguageOptions and TargetInfo. I am not sure if this makes things overly complicated.

I would expect target specific address space casts in C++ to be rare cases since it is not part of language standard. In most cases users would just use generic pointer. When they do use explicit address space cast, I expect they understand what they are doing. Then clang just do not do any target-specific promotion and treat it as reinterpret cast. Promotion is only allowed by language rules e.g. those defined by OpenCL.

[rjmccall]

lib/Sema/SemaOverload.cpp

3150 ↗

(On Diff #155353)

Well, address spaces in "pure" C/C++ are specified by ISO/IEC TR 18037, the Embedded C specification, which does cover the possibility of target-specific overlapping address spaces. Since we don't support those yet, that's fine, I'm not going to ask you to add that infrastructure; but I do think you should at least handle the language-specific overlap rules here, and in particular you should handle them by calling one of those functions you mentioned, so that if someone *does* want to implement target-specific overlapping address spaces, it's obvious that this is one of the places that needs to be fixed and tested.

[yaxunl]

Revised by John's comments.

[rjmccall]

Thanks, looks good.

cfe/trunk/lib/Sema/SemaCast.cpp

static bool fixOverloadedReinterpretCastExpr(Sema &Self, QualType DestType,
   // preserves Result.
   Result = E;
   if (!Self.resolveAndFixAddressOfOnlyViableOverloadCandidate(
           Result, /*DoFunctionPointerConversion=*/true))
     return false;
   return Result.isUsable();
 }
 
+static bool IsAddressSpaceConversion(QualType SrcType, QualType DestType) {
+  return SrcType->isPointerType() && DestType->isPointerType() &&
+         SrcType->getAs<PointerType>()->getPointeeType().getAddressSpace() !=
+             DestType->getAs<PointerType>()->getPointeeType().getAddressSpace();
+}
+
 static TryCastResult TryReinterpretCast(Sema &Self, ExprResult &SrcExpr,
                                         QualType DestType, bool CStyle,
                                         SourceRange OpRange,
                                         unsigned &msg,
                                         CastKind &Kind) {
   bool IsLValueCast = false;
   
   DestType = Self.Context.getCanonicalType(DestType);
static TryCastResult TryReinterpretCast(Sema &Self, ExprResult &SrcExpr,
   } else if (DestType->isObjCObjectPointerType()) {
     Kind = Self.PrepareCastToObjCObjectPointer(SrcExpr);
   } else if (DestType->isBlockPointerType()) {
     if (!SrcType->isBlockPointerType()) {
       Kind = CK_AnyPointerToBlockPointerCast;
     } else {
       Kind = CK_BitCast;
     }
+  } else if (IsAddressSpaceConversion(SrcType, DestType)) {
+    Kind = CK_AddressSpaceConversion;
   } else {
     Kind = CK_BitCast;
   }
 
   // Any pointer can be cast to an Objective-C pointer type with a C-style
   // cast.
   if (CStyle && DestType->isObjCObjectPointerType()) {
     return SuccessResult;

cfe/trunk/lib/Sema/SemaOverload.cpp

if (!CStyle && FromQuals.getCVRQualifiers() != ToQuals.getCVRQualifiers()
       return false;
 
     // Keep track of whether all prior cv-qualifiers in the "to" type
     // include const.
     PreviousToQualsIncludeConst
       = PreviousToQualsIncludeConst && ToQuals.hasConst();
   }
 
+  // Allows address space promotion by language rules implemented in
+  // Type::Qualifiers::isAddressSpaceSupersetOf.
+  Qualifiers FromQuals = FromType.getQualifiers();
+  Qualifiers ToQuals = ToType.getQualifiers();
+  if (!ToQuals.isAddressSpaceSupersetOf(FromQuals) &&
+      !FromQuals.isAddressSpaceSupersetOf(ToQuals)) {
+    return false;
+  }
+
   // We are left with FromType and ToType being the pointee types
   // after unwrapping the original FromType and ToType the same number
   // of types. If we unwrapped any pointers, and if FromType and
   // ToType have the same unqualified type (since we checked
   // qualifiers above), then this is a qualification conversion.
   return UnwrappedAnyPointer && Context.hasSameUnqualifiedType(FromType,ToType);
 }
 

cfe/trunk/test/CodeGenCXX/address-space-cast.cpp

+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -emit-llvm -o - | FileCheck %s
+
+#define __private__ __attribute__((address_space(5)))
+
+void func_pchar(__private__ char *x);
+
+void test_cast(char *gen_ptr) {
+  // CHECK: %[[cast:.*]] = addrspacecast i8* %{{.*}} to i8 addrspace(5)*
+  // CHECK-NEXT: store i8 addrspace(5)* %[[cast]]
+  __private__ char *priv_ptr = (__private__ char *)gen_ptr;
+
+  // CHECK: %[[cast:.*]] = addrspacecast i8* %{{.*}} to i8 addrspace(5)*
+  // CHECK-NEXT: call void @_Z10func_pcharPU3AS5c(i8 addrspace(5)* %[[cast]])
+  func_pchar((__private__ char *)gen_ptr);
+}