<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, 20 Jul 2018 at 12:00, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, 20 Jul 2018 at 04:38, Yaxun Liu via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: yaxunl<br>
Date: Fri Jul 20 04:32:51 2018<br>
New Revision: 337540<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=337540&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=337540&view=rev</a><br>
Log:<br>
Sema: Fix explicit address space cast in C++<br>
<br>
Currently clang does not allow implicit cast of a pointer to a pointer type<br>
in different address space but allows C-style cast of a pointer to a pointer<br>
type in different address space. However, there is a bug in Sema causing<br>
incorrect Cast Expr in AST for the latter case, which in turn results in<br>
invalid LLVM IR in codegen.<br>
<br>
This is because Sema::IsQualificationConversion returns true for a cast of<br>
pointer to a pointer type in different address space, which in turn allows<br>
a standard conversion and results in a cast expression with no op in AST.<br>
<br>
This patch fixes that by let Sema::IsQualificationConversion returns false<br>
for a cast of pointer to a pointer type in different address space, which<br>
in turn disallows standard conversion, implicit cast, and static cast.<br>
Finally it results in an reinterpret cast and correct conversion kind is set.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D49294" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49294</a><br>
<br>
Added:<br>
cfe/trunk/test/CodeGenCXX/address-space-cast.cpp<br>
Modified:<br>
cfe/trunk/lib/Sema/SemaCast.cpp<br>
cfe/trunk/lib/Sema/SemaOverload.cpp<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaCast.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCast.cpp?rev=337540&r1=337539&r2=337540&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCast.cpp?rev=337540&r1=337539&r2=337540&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaCast.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaCast.cpp Fri Jul 20 04:32:51 2018<br>
@@ -1955,6 +1955,12 @@ static bool fixOverloadedReinterpretCast<br>
return Result.isUsable();<br>
}<br>
<br>
+static bool IsAddressSpaceConversion(QualType SrcType, QualType DestType) {<br>
+ return SrcType->isPointerType() && DestType->isPointerType() &&<br>
+ SrcType->getAs<PointerType>()->getPointeeType().getAddressSpace() !=<br>
+ DestType->getAs<PointerType>()->getPointeeType().getAddressSpace();<br>
+}<br>
+<br>
static TryCastResult TryReinterpretCast(Sema &Self, ExprResult &SrcExpr,<br>
QualType DestType, bool CStyle,<br>
SourceRange OpRange,<br>
@@ -2198,6 +2204,8 @@ static TryCastResult TryReinterpretCast(<br>
} else {<br>
Kind = CK_BitCast;<br>
}<br>
+ } else if (IsAddressSpaceConversion(SrcType, DestType)) {<br>
+ Kind = CK_AddressSpaceConversion;<br></blockquote><div><br></div><div>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.</div></div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>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.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
} else {<br>
Kind = CK_BitCast;<br>
}<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaOverload.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=337540&r1=337539&r2=337540&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=337540&r1=337539&r2=337540&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaOverload.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaOverload.cpp Fri Jul 20 04:32:51 2018<br>
@@ -3150,6 +3150,15 @@ Sema::IsQualificationConversion(QualType<br>
= PreviousToQualsIncludeConst && ToQuals.hasConst();<br>
}<br>
<br>
+ // Allows address space promotion by language rules implemented in<br>
+ // Type::Qualifiers::isAddressSpaceSupersetOf.<br>
+ Qualifiers FromQuals = FromType.getQualifiers();<br>
+ Qualifiers ToQuals = ToType.getQualifiers();<br>
+ if (!ToQuals.isAddressSpaceSupersetOf(FromQuals) &&<br>
+ !FromQuals.isAddressSpaceSupersetOf(ToQuals)) {<br></blockquote></div></div></blockquote><div><br></div><div>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:</div><div><br></div><div> "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."</div><div><br></div><div>... 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:</div><div><br></div><div> char *p;</div><div> __private__ char *q = p;</div><div><br></div><div>(with no cast) in C++.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ return false;<br>
+ }<br>
+<br>
// We are left with FromType and ToType being the pointee types<br>
// after unwrapping the original FromType and ToType the same number<br>
// of types. If we unwrapped any pointers, and if FromType and<br>
<br>
Added: cfe/trunk/test/CodeGenCXX/address-space-cast.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/address-space-cast.cpp?rev=337540&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/address-space-cast.cpp?rev=337540&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/CodeGenCXX/address-space-cast.cpp (added)<br>
+++ cfe/trunk/test/CodeGenCXX/address-space-cast.cpp Fri Jul 20 04:32:51 2018<br>
@@ -0,0 +1,15 @@<br>
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -emit-llvm -o - | FileCheck %s<br>
+<br>
+#define __private__ __attribute__((address_space(5)))<br>
+<br>
+void func_pchar(__private__ char *x);<br>
+<br>
+void test_cast(char *gen_ptr) {<br>
+ // CHECK: %[[cast:.*]] = addrspacecast i8* %{{.*}} to i8 addrspace(5)*<br>
+ // CHECK-NEXT: store i8 addrspace(5)* %[[cast]]<br>
+ __private__ char *priv_ptr = (__private__ char *)gen_ptr;<br>
+<br>
+ // CHECK: %[[cast:.*]] = addrspacecast i8* %{{.*}} to i8 addrspace(5)*<br>
+ // CHECK-NEXT: call void @_Z10func_pcharPU3AS5c(i8 addrspace(5)* %[[cast]])<br>
+ func_pchar((__private__ char *)gen_ptr);<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>
</blockquote></div></div>