[PATCH] D49294: Sema: Fix explicit address space cast in C++
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337540: Sema: Fix explicit address space cast in C++
(authored by yaxunl, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D49294?vs=156403=156469#toc
Repository:
rL LLVM
https://reviews.llvm.org/D49294
Files:
cfe/trunk/lib/Sema/SemaCast.cpp
cfe/trunk/lib/Sema/SemaOverload.cpp
cfe/trunk/test/CodeGenCXX/address-space-cast.cpp
Index: cfe/trunk/test/CodeGenCXX/address-space-cast.cpp
===
--- cfe/trunk/test/CodeGenCXX/address-space-cast.cpp
+++ cfe/trunk/test/CodeGenCXX/address-space-cast.cpp
@@ -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);
+}
Index: cfe/trunk/lib/Sema/SemaCast.cpp
===
--- cfe/trunk/lib/Sema/SemaCast.cpp
+++ cfe/trunk/lib/Sema/SemaCast.cpp
@@ -1955,6 +1955,12 @@
return Result.isUsable();
}
+static bool IsAddressSpaceConversion(QualType SrcType, QualType DestType) {
+ return SrcType->isPointerType() && DestType->isPointerType() &&
+ SrcType->getAs()->getPointeeType().getAddressSpace() !=
+
DestType->getAs()->getPointeeType().getAddressSpace();
+}
+
static TryCastResult TryReinterpretCast(Sema , ExprResult ,
QualType DestType, bool CStyle,
SourceRange OpRange,
@@ -2198,6 +2204,8 @@
} else {
Kind = CK_BitCast;
}
+ } else if (IsAddressSpaceConversion(SrcType, DestType)) {
+Kind = CK_AddressSpaceConversion;
} else {
Kind = CK_BitCast;
}
Index: cfe/trunk/lib/Sema/SemaOverload.cpp
===
--- cfe/trunk/lib/Sema/SemaOverload.cpp
+++ cfe/trunk/lib/Sema/SemaOverload.cpp
@@ -3150,6 +3150,15 @@
= 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
Index: cfe/trunk/test/CodeGenCXX/address-space-cast.cpp
===
--- cfe/trunk/test/CodeGenCXX/address-space-cast.cpp
+++ cfe/trunk/test/CodeGenCXX/address-space-cast.cpp
@@ -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);
+}
Index: cfe/trunk/lib/Sema/SemaCast.cpp
===
--- cfe/trunk/lib/Sema/SemaCast.cpp
+++ cfe/trunk/lib/Sema/SemaCast.cpp
@@ -1955,6 +1955,12 @@
return Result.isUsable();
}
+static bool IsAddressSpaceConversion(QualType SrcType, QualType DestType) {
+ return SrcType->isPointerType() && DestType->isPointerType() &&
+ SrcType->getAs()->getPointeeType().getAddressSpace() !=
+ DestType->getAs()->getPointeeType().getAddressSpace();
+}
+
static TryCastResult TryReinterpretCast(Sema , ExprResult ,
QualType DestType, bool CStyle,
SourceRange OpRange,
@@ -2198,6 +2204,8 @@
} else {
Kind = CK_BitCast;
}
+ } else if (IsAddressSpaceConversion(SrcType, DestType)) {
+Kind = CK_AddressSpaceConversion;
} else {
Kind = CK_BitCast;
}
Index: cfe/trunk/lib/Sema/SemaOverload.cpp
===
[PATCH] D49294: Sema: Fix explicit address space cast in C++
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, looks good. https://reviews.llvm.org/D49294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49294: Sema: Fix explicit address space cast in C++
yaxunl updated this revision to Diff 156403.
yaxunl marked 5 inline comments as done.
yaxunl added a comment.
Revised by John's comments.
https://reviews.llvm.org/D49294
Files:
lib/Sema/SemaCast.cpp
lib/Sema/SemaOverload.cpp
test/CodeGenCXX/address-space-cast.cpp
Index: test/CodeGenCXX/address-space-cast.cpp
===
--- /dev/null
+++ test/CodeGenCXX/address-space-cast.cpp
@@ -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);
+}
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -3143,6 +3143,15 @@
= 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
Index: lib/Sema/SemaCast.cpp
===
--- lib/Sema/SemaCast.cpp
+++ lib/Sema/SemaCast.cpp
@@ -1922,6 +1922,12 @@
return Result.isUsable();
}
+static bool IsAddressSpaceConversion(QualType SrcType, QualType DestType) {
+ return SrcType->isPointerType() && DestType->isPointerType() &&
+ SrcType->getAs()->getPointeeType().getAddressSpace() !=
+
DestType->getAs()->getPointeeType().getAddressSpace();
+}
+
static TryCastResult TryReinterpretCast(Sema , ExprResult ,
QualType DestType, bool CStyle,
SourceRange OpRange,
@@ -2165,6 +2171,8 @@
} else {
Kind = CK_BitCast;
}
+ } else if (IsAddressSpaceConversion(SrcType, DestType)) {
+Kind = CK_AddressSpaceConversion;
} else {
Kind = CK_BitCast;
}
Index: test/CodeGenCXX/address-space-cast.cpp
===
--- /dev/null
+++ test/CodeGenCXX/address-space-cast.cpp
@@ -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);
+}
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -3143,6 +3143,15 @@
= 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
Index: lib/Sema/SemaCast.cpp
===
--- lib/Sema/SemaCast.cpp
+++ lib/Sema/SemaCast.cpp
@@ -1922,6 +1922,12 @@
return Result.isUsable();
}
+static bool IsAddressSpaceConversion(QualType SrcType, QualType DestType) {
+ return SrcType->isPointerType() && DestType->isPointerType() &&
+ SrcType->getAs()->getPointeeType().getAddressSpace() !=
+ DestType->getAs()->getPointeeType().getAddressSpace();
+}
+
static TryCastResult TryReinterpretCast(Sema , ExprResult ,
QualType DestType, bool
[PATCH] D49294: Sema: Fix explicit address space cast in C++
rjmccall added inline comments. Comment at: lib/Sema/SemaOverload.cpp:3150 + !getLangOpts().OpenCLCPlusPlus) +return false; + yaxunl wrote: > rjmccall wrote: > > yaxunl wrote: > > > rjmccall wrote: > > > > It's not really OpenCL C++ that's special here, it's the possibility of > > > > promotions between address spaces. > > > 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? > > 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. > 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. > 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. https://reviews.llvm.org/D49294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49294: Sema: Fix explicit address space cast in C++
yaxunl marked an inline comment as done. yaxunl added inline comments. Comment at: lib/Sema/SemaOverload.cpp:3150 + !getLangOpts().OpenCLCPlusPlus) +return false; + rjmccall wrote: > yaxunl wrote: > > rjmccall wrote: > > > It's not really OpenCL C++ that's special here, it's the possibility of > > > promotions between address spaces. > > 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? > 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. 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. https://reviews.llvm.org/D49294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49294: Sema: Fix explicit address space cast in C++
rjmccall added inline comments. Comment at: lib/Sema/SemaOverload.cpp:3150 + !getLangOpts().OpenCLCPlusPlus) +return false; + yaxunl wrote: > rjmccall wrote: > > It's not really OpenCL C++ that's special here, it's the possibility of > > promotions between address spaces. > 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? 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. https://reviews.llvm.org/D49294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49294: Sema: Fix explicit address space cast in C++
yaxunl added inline comments. Comment at: lib/Sema/SemaOverload.cpp:3150 + !getLangOpts().OpenCLCPlusPlus) +return false; + rjmccall wrote: > It's not really OpenCL C++ that's special here, it's the possibility of > promotions between address spaces. 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? https://reviews.llvm.org/D49294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49294: Sema: Fix explicit address space cast in C++
rjmccall added inline comments.
Comment at: lib/Sema/SemaCast.cpp:2171
+ ->getPointeeType()
+ .getAddressSpace()) {
+Kind = CK_AddressSpaceConversion;
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?
Comment at: lib/Sema/SemaOverload.cpp:3150
+ !getLangOpts().OpenCLCPlusPlus)
+return false;
+
It's not really OpenCL C++ that's special here, it's the possibility of
promotions between address spaces.
https://reviews.llvm.org/D49294
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49294: Sema: Fix explicit address space cast in C++
yaxunl created this revision.
yaxunl added a reviewer: rjmccall.
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.
https://reviews.llvm.org/D49294
Files:
lib/Sema/SemaCast.cpp
lib/Sema/SemaOverload.cpp
test/CodeGenCXX/address-space-cast.cpp
Index: test/CodeGenCXX/address-space-cast.cpp
===
--- /dev/null
+++ test/CodeGenCXX/address-space-cast.cpp
@@ -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(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);
+}
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -3143,6 +3143,12 @@
= PreviousToQualsIncludeConst && ToQuals.hasConst();
}
+ // ToDo: Add more detailed control for implicit address space casting for
+ // OpenCL C++.
+ if (FromType.getAddressSpace() != ToType.getAddressSpace() &&
+ !getLangOpts().OpenCLCPlusPlus)
+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
Index: lib/Sema/SemaCast.cpp
===
--- lib/Sema/SemaCast.cpp
+++ lib/Sema/SemaCast.cpp
@@ -2163,6 +2163,13 @@
} else {
Kind = CK_BitCast;
}
+ } else if (SrcType->isPointerType() && DestType->isPointerType() &&
+ SrcType->getAs()
+ ->getPointeeType()
+ .getAddressSpace() != DestType->getAs()
+ ->getPointeeType()
+ .getAddressSpace()) {
+Kind = CK_AddressSpaceConversion;
} else {
Kind = CK_BitCast;
}
Index: test/CodeGenCXX/address-space-cast.cpp
===
--- /dev/null
+++ test/CodeGenCXX/address-space-cast.cpp
@@ -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(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);
+}
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -3143,6 +3143,12 @@
= PreviousToQualsIncludeConst && ToQuals.hasConst();
}
+ // ToDo: Add more detailed control for implicit address space casting for
+ // OpenCL C++.
+ if (FromType.getAddressSpace() != ToType.getAddressSpace() &&
+ !getLangOpts().OpenCLCPlusPlus)
+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
Index: lib/Sema/SemaCast.cpp
===
--- lib/Sema/SemaCast.cpp
+++ lib/Sema/SemaCast.cpp
@@ -2163,6 +2163,13 @@
} else {
Kind = CK_BitCast;
}
+ } else if (SrcType->isPointerType() && DestType->isPointerType() &&
+ SrcType->getAs()
+ ->getPointeeType()
+
