r337540 - Sema: Fix explicit address space cast in C++

Richard Smith via cfe-commits cfe-commits at lists.llvm.org
Fri Jul 20 12:29:29 PDT 2018


On Fri, 20 Jul 2018 at 12:00, Richard Smith <richard at metafoo.co.uk> wrote:

> On Fri, 20 Jul 2018 at 04:38, Yaxun Liu via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: yaxunl
>> Date: Fri Jul 20 04:32:51 2018
>> New Revision: 337540
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=337540&view=rev
>> Log:
>> Sema: Fix explicit address space cast in C++
>>
>> 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.
>>
>> Differential Revision: https://reviews.llvm.org/D49294
>>
>> Added:
>>     cfe/trunk/test/CodeGenCXX/address-space-cast.cpp
>> Modified:
>>     cfe/trunk/lib/Sema/SemaCast.cpp
>>     cfe/trunk/lib/Sema/SemaOverload.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaCast.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCast.cpp?rev=337540&r1=337539&r2=337540&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaCast.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaCast.cpp Fri Jul 20 04:32:51 2018
>> @@ -1955,6 +1955,12 @@ static bool fixOverloadedReinterpretCast
>>    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,
>> @@ -2198,6 +2204,8 @@ static TryCastResult TryReinterpretCast(
>>      } else {
>>        Kind = CK_BitCast;
>>      }
>> +  } else if (IsAddressSpaceConversion(SrcType, DestType)) {
>> +    Kind = CK_AddressSpaceConversion;
>>
>
> This seems wrong to me. A reinterpret_cast from a pointer to one address
> space into a pointer to a different address space should be a bit cast, not
> an address space conversion.
>

Hmm. I suppose we have a choice: either reinterpret_cast always means
'reinterpret these bits as this other type' (and there are C-style casts
that cannot be expressed in terms of named casts, violating the usual C++
rules for C-style casts) or reinterpret_cast between pointers always gives
you a pointer to the same byte of storage and an address space bitcast
requires casting via an integral type (violating the normal behavior that
reinterpret_cast reinterprets the representation and doesn't change the
value). These options both seem unsatisfying, but what you have here
(reinterpret_cast attempts to form a pointer to the same byte) is
definitely the more useful of those two options.

Do any of our supported language extensions actually say what named casts
in C++ do when attempting to cast between address spaces? The OpenCL 2.2
C++ extensions specification doesn't add address space qualifiers, so it
doesn't have an answer for us.

   } else {
>>      Kind = CK_BitCast;
>>    }
>>
>> Modified: cfe/trunk/lib/Sema/SemaOverload.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=337540&r1=337539&r2=337540&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaOverload.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaOverload.cpp Fri Jul 20 04:32:51 2018
>> @@ -3150,6 +3150,15 @@ Sema::IsQualificationConversion(QualType
>>        = 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)) {
>>
>
Is this right? Qualification conversions are usually only permitted for
"guaranteed-safe" conversions, which would be cases where the target
address space is a superset of the source address space only. And OpenCL
2.2's C extensions specification (section 1.5.6, changes to 6.3.2.3) says:

  "For any qualifier q, a pointer to a non-q-qualified type may be
converted to a pointer to the q-qualified version of the type (but with the
same address-space qualifier or the generic address space); the values
stored in the original and converted pointers shall compare equal."

... which looks to me like OpenCL only permits going from subset to
superset address space in C's equivalent to a qualification conversion. I
don't think we want to support:

  char *p;
  __private__ char *q = p;

(with no cast) in C++.

+    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
>>
>> Added: cfe/trunk/test/CodeGenCXX/address-space-cast.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/address-space-cast.cpp?rev=337540&view=auto
>>
>> ==============================================================================
>> --- cfe/trunk/test/CodeGenCXX/address-space-cast.cpp (added)
>> +++ cfe/trunk/test/CodeGenCXX/address-space-cast.cpp Fri Jul 20 04:32:51
>> 2018
>> @@ -0,0 +1,15 @@
>> +// 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);
>> +}
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180720/1a7ed30b/attachment.html>


More information about the cfe-commits mailing list