[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
This revision was automatically updated to reflect the committed changes.
Closed by commit rL315668: [OpenCL] Add LangAS::opencl_private to represent
private address space in AST (authored by yaxunl).
Changed prior to commit:
https://reviews.llvm.org/D35082?vs=118813=118882#toc
Repository:
rL LLVM
https://reviews.llvm.org/D35082
Files:
cfe/trunk/include/clang/Basic/AddressSpaces.h
cfe/trunk/lib/AST/ASTContext.cpp
cfe/trunk/lib/AST/Expr.cpp
cfe/trunk/lib/AST/ItaniumMangle.cpp
cfe/trunk/lib/AST/TypePrinter.cpp
cfe/trunk/lib/Basic/Targets/AMDGPU.cpp
cfe/trunk/lib/Basic/Targets/NVPTX.h
cfe/trunk/lib/Basic/Targets/SPIR.h
cfe/trunk/lib/Basic/Targets/TCE.h
cfe/trunk/lib/CodeGen/CGDecl.cpp
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/CodeGenOpenCL/address-spaces-mangling.cl
cfe/trunk/test/CodeGenOpenCL/address-spaces.cl
cfe/trunk/test/SemaOpenCL/address-spaces.cl
cfe/trunk/test/SemaOpenCL/cl20-device-side-enqueue.cl
cfe/trunk/test/SemaOpenCL/extern.cl
cfe/trunk/test/SemaOpenCL/storageclass-cl20.cl
cfe/trunk/test/SemaOpenCL/storageclass.cl
cfe/trunk/test/SemaTemplate/address_space-dependent.cpp
Index: cfe/trunk/include/clang/Basic/AddressSpaces.h
===
--- cfe/trunk/include/clang/Basic/AddressSpaces.h
+++ cfe/trunk/include/clang/Basic/AddressSpaces.h
@@ -25,16 +25,17 @@
///
enum ID {
// The default value 0 is the value used in QualType for the the situation
- // where there is no address space qualifier. For most languages, this also
- // corresponds to the situation where there is no address space qualifier in
- // the source code, except for OpenCL, where the address space value 0 in
- // QualType represents private address space in OpenCL source code.
+ // where there is no address space qualifier.
Default = 0,
// OpenCL specific address spaces.
+ // In OpenCL each l-value must have certain non-default address space, each
+ // r-value must have no address space (i.e. the default address space). The
+ // pointee of a pointer must have non-default address space.
opencl_global,
opencl_local,
opencl_constant,
+ opencl_private,
opencl_generic,
// CUDA specific address spaces.
Index: cfe/trunk/test/CodeGenOpenCL/address-spaces.cl
===
--- cfe/trunk/test/CodeGenOpenCL/address-spaces.cl
+++ cfe/trunk/test/CodeGenOpenCL/address-spaces.cl
@@ -7,6 +7,24 @@
// RUN: %clang_cc1 %s -O0 -triple amdgcn-mesa-mesa3d -emit-llvm -o - | FileCheck --check-prefixes=CHECK,SPIR %s
// RUN: %clang_cc1 %s -O0 -triple r600-- -emit-llvm -o - | FileCheck --check-prefixes=CHECK,SPIR %s
+// SPIR: %struct.S = type { i32, i32, i32* }
+// CL20SPIR: %struct.S = type { i32, i32, i32 addrspace(4)* }
+struct S {
+ int x;
+ int y;
+ int *z;
+};
+
+// CL20-DAG: @g_extern_var = external addrspace(1) global float
+// CL20-DAG: @l_extern_var = external addrspace(1) global float
+// CL20-DAG: @test_static.l_static_var = internal addrspace(1) global float 0.00e+00
+// CL20-DAG: @g_static_var = internal addrspace(1) global float 0.00e+00
+
+#ifdef CL20
+// CL20-DAG: @g_s = common addrspace(1) global %struct.S zeroinitializer
+struct S g_s;
+#endif
+
// SPIR: i32* %arg
// GIZ: i32 addrspace(5)* %arg
void f__p(__private int *arg) {}
@@ -58,3 +76,52 @@
// CL20-DAG: @f.ii = internal addrspace(1) global i32 0
#endif
}
+
+typedef int int_td;
+typedef int *intp_td;
+// SPIR: define void @test_typedef(i32 addrspace(1)* %x, i32 addrspace(2)* %y, i32* %z)
+void test_typedef(global int_td *x, constant int_td *y, intp_td z) {
+ *x = *y;
+ *z = 0;
+}
+
+// SPIR: define void @test_struct()
+void test_struct() {
+ // SPIR: %ps = alloca %struct.S*
+ // CL20SPIR: %ps = alloca %struct.S addrspace(4)*
+ struct S *ps;
+ // SPIR: store i32 0, i32* %x
+ // CL20SPIR: store i32 0, i32 addrspace(4)* %x
+ ps->x = 0;
+#ifdef CL20
+ // CL20SPIR: store i32 0, i32 addrspace(1)* getelementptr inbounds (%struct.S, %struct.S addrspace(1)* @g_s, i32 0, i32 0)
+ g_s.x = 0;
+#endif
+}
+
+// SPIR-LABEL: define void @test_void_par()
+void test_void_par(void) {}
+
+// SPIR-LABEL: define i32 @test_func_return_type()
+int test_func_return_type(void) {
+ return 0;
+}
+
+#ifdef CL20
+extern float g_extern_var;
+
+// CL20-LABEL: define {{.*}}void @test_extern(
+kernel void test_extern(global float *buf) {
+ extern float l_extern_var;
+ buf[0] += g_extern_var + l_extern_var;
+}
+
+static float g_static_var;
+
+// CL20-LABEL: define {{.*}}void @test_static(
+kernel void test_static(global float *buf) {
+ static float l_static_var;
+ buf[0] += g_static_var + l_static_var;
+}
+
+#endif
Index: cfe/trunk/test/CodeGenOpenCL/address-spaces-mangling.cl
===
--- cfe/trunk/test/CodeGenOpenCL/address-spaces-mangling.cl
+++
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment. A few minor comments; feel free to commit after addressing them. Comment at: include/clang/Basic/AddressSpaces.h:34 // OpenCL specific address spaces. opencl_global, yaxunl wrote: > rjmccall wrote: > > I think you need a real comment about the design of OpenCL address spaces > > here. Specifically, it is important to note that OpenCL no longer uses > > LangAS::Default for anything except r-values. > Will do. Thanks, WFM. Comment at: lib/AST/TypePrinter.cpp:1703 OS << "__shared"; break; default: Please add the case here, even if it's unreachable for now. Comment at: lib/Sema/SemaType.cpp:5754 default: assert(Attr.getKind() == AttributeList::AT_OpenCLPrivateAddressSpace); + ASIdx = LangAS::opencl_private; break; Please just make this a case, and then the default can be llvm_unreachable. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl updated this revision to Diff 118813.
yaxunl marked 7 inline comments as done.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Separate implicit addr space flag to another patch as John suggested.
This patch only introduces the private addr space but does not print it.
https://reviews.llvm.org/D35082
Files:
include/clang/Basic/AddressSpaces.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/TypePrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/Basic/Targets/NVPTX.h
lib/Basic/Targets/SPIR.h
lib/Basic/Targets/TCE.h
lib/CodeGen/CGDecl.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCL/address-spaces-mangling.cl
test/CodeGenOpenCL/address-spaces.cl
test/SemaOpenCL/address-spaces.cl
test/SemaOpenCL/cl20-device-side-enqueue.cl
test/SemaOpenCL/extern.cl
test/SemaOpenCL/storageclass-cl20.cl
test/SemaOpenCL/storageclass.cl
test/SemaTemplate/address_space-dependent.cpp
Index: test/SemaTemplate/address_space-dependent.cpp
===
--- test/SemaTemplate/address_space-dependent.cpp
+++ test/SemaTemplate/address_space-dependent.cpp
@@ -43,7 +43,7 @@
template
void tooBig() {
- __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388599)}}
+ __attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388598)}}
}
template
@@ -101,7 +101,7 @@
car<1, 2, 3>(); // expected-note {{in instantiation of function template specialization 'car<1, 2, 3>' requested here}}
HasASTemplateFields<1> HASTF;
neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
- correct<0x77>();
+ correct<0x76>();
tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650>' requested here}}
__attribute__((address_space(1))) char *x;
Index: test/SemaOpenCL/storageclass.cl
===
--- test/SemaOpenCL/storageclass.cl
+++ test/SemaOpenCL/storageclass.cl
@@ -5,6 +5,20 @@
int G3 = 0;// expected-error{{program scope variable must reside in constant address space}}
global int G4 = 0; // expected-error{{program scope variable must reside in constant address space}}
+static float g_implicit_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static constant float g_constant_static_var = 0;
+static global float g_global_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static local float g_local_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static private float g_private_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static generic float g_generic_static_var = 0; // expected-error{{OpenCL version 1.2 does not support the 'generic' type qualifier}} // expected-error {{program scope variable must reside in constant address space}}
+
+extern float g_implicit_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern constant float g_constant_extern_var;
+extern global float g_global_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern local float g_local_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern private float g_private_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern generic float g_generic_extern_var; // expected-error{{OpenCL version 1.2 does not support the 'generic' type qualifier}} // expected-error {{extern variable must reside in constant address space}}
+
void kernel foo(int x) {
// static is not allowed at local scope before CL2.0
static int S1 = 5; // expected-error{{variables in function scope cannot be declared static}}
@@ -45,10 +59,17 @@
__attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
}
+ static float l_implicit_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static constant float l_constant_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static global float l_global_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static local float l_local_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static private float l_private_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static generic float
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment. In https://reviews.llvm.org/D35082#895312, @yaxunl wrote: > Thanks. I will separate the implicit addr space flag to another patch. Thanks, appreciated. Comment at: include/clang/AST/Type.h:562 + static const uint32_t IMask = 0x200; + static const uint32_t IShift = 9; static const uint32_t AddressSpaceMask = yaxunl wrote: > rjmccall wrote: > > "I" is not an appropriate abbreviation for "AddressSpaceImplicit". > Will change it to ImplictAddrSpace. Sounds good. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl marked 2 inline comments as done. yaxunl added a comment. In https://reviews.llvm.org/D35082#895230, @rjmccall wrote: > It sounds like there's agreement about the basic technical direction of > introducing LangAS::opencl_private. Please introduce > isAddressSpaceImplicit() in a different patch and make this patch just about > the introduction of LangAS::opencl_private. You can have the pretty-printer > just ignore __private for now, which should avoid gratuitous diagnostic > changes. > > I would like you to investigate using AttributedType for the pretty-printing > and address-space semantic checks before adding isAddressSpaceImplicit(). Thanks. I will separate the implicit addr space flag to another patch. Comment at: include/clang/AST/Type.h:562 + static const uint32_t IMask = 0x200; + static const uint32_t IShift = 9; static const uint32_t AddressSpaceMask = rjmccall wrote: > "I" is not an appropriate abbreviation for "AddressSpaceImplicit". Will change it to ImplictAddrSpace. Comment at: include/clang/Basic/AddressSpaces.h:34 // OpenCL specific address spaces. opencl_global, rjmccall wrote: > I think you need a real comment about the design of OpenCL address spaces > here. Specifically, it is important to note that OpenCL no longer uses > LangAS::Default for anything except r-values. Will do. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment. It sounds like there's agreement about the basic technical direction of introducing LangAS::opencl_private. Please introduce isAddressSpaceImplicit() in a different patch and make this patch just about the introduction of LangAS::opencl_private. You can have the pretty-printer just ignore __private for now, which should avoid gratuitous diagnostic changes. I would like you to investigate using AttributedType for the pretty-printing and address-space semantic checks before adding isAddressSpaceImplicit(). Comment at: include/clang/AST/Type.h:562 + static const uint32_t IMask = 0x200; + static const uint32_t IShift = 9; static const uint32_t AddressSpaceMask = "I" is not an appropriate abbreviation for "AddressSpaceImplicit". Comment at: include/clang/Basic/AddressSpaces.h:34 // OpenCL specific address spaces. opencl_global, I think you need a real comment about the design of OpenCL address spaces here. Specifically, it is important to note that OpenCL no longer uses LangAS::Default for anything except r-values. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl added a comment. If there is no other issues. May I commit this patch now? Thanks. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added a comment. > (*) I know that you aren't considering OpenCL C++ yet, but often these > representation/model questions are easier to answer when thinking about C++ > instead of C because type differences are so much more directly important in > C++. In OpenCL C++, I assume it will be important to distinguish between > and <__private int> as template arguments. That tells us straight up > that __private needs to be represented differently from a lack of qualifier. > Note that the result, where certain type representations (e.g. an unqualified > int*) become basically meaningless and should never be constructed by Sema, > is already precedented in Clang: ObjC ARC does the same thing to unqualified > ObjC pointer types, like id*. It's pretty complicated in OpenCL as `int` and `private int` won't be different, but `int*` and `private int*` would be different only in OpenCL2.0. The rules are pretty convoluted and I have summarized them in the table earlier: https://reviews.llvm.org/D35082#inline-327303. The issue is that there is not anything like this in other languages and therefore we have this issue trying to fit this concept with something similar but not exactly the same. I thought you and Sam decided to use implicit AS flag to make printing messages consistent with the original source code. I am fine with this approach, however, I would prefer to just keep no AS qualifier in the default AS mode instead of deducing it during parcing in `processTypeAttrs`, and only to add the AS to the IR at the end of the CodeGen phase. I think it would be a lot easier to understand. However, as Sam has pointed out a lot of code in Sema has been written assuming the deduction of AS and is using AS qualifiers explicitly and therefore it seems like it would be a bigger change to go for. At the same type we have refactored the deduction of default AS now in the parcing phase and it looks a lot better than I thought it would be. So I don't mind if we continue this way. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment. In https://reviews.llvm.org/D35082#890359, @Anastasia wrote: > In https://reviews.llvm.org/D35082#890162, @rjmccall wrote: > > > Okay. I think I see your point about why it would be better to have a > > canonical __private address space that is different from the implicit > > address space 0. I assume this means that there should basically never be > > pointers, references, or l-values in address space 0 in OpenCL. > > > If you mean 0 is `private` address space then no. There can be private AS > pointers. But if you mean 0 is no address space then this doesn't exist in > OpenCL. I think the right design in the first place would have been to keep > address spaces in AST just as they are in the source code and add explicit > address spaces to all types in IR instead. In this case absence of any > address spaces in AST would signal implicit AS to be used. This would make > some Sema checks a bit more complicated though, but the design would become a > lot cleaner. Unfortunately I think it would not be worth doing this big > change now. My understanding is that that is exactly what the majority of this patch is trying to achieve: making __private its own address space, separate from address space 0. The patch is certainly introducing a new address-space enumerator for __private; if that's meant to be treated as canonically equal to address space 0, well, (1) this patch doesn't seem to add any of the APIs that I'd expect would be required to make that work, like a way to canonicalize an address space or ask if two address spaces are canonically the same, and (2) that would be a major new complexity in the representation that we wouldn't welcome anyway. The patch is also tracking explicitness of address spaces in the non-canonical type system, but the majority of the patch is spent dealing with the consequences of splitting __private away from address space 0. Yaxun has convinced me that it's important to represent a __private-qualified type differently from an unqualified type.(*) It seems that you agree, but you're concerned about how that will affect your schedule. I can't speak to your schedule; it seems to me that you and Yaxun need to figure that out privately. If you decide not to separate them yet, then the majority of this patch needs to be put on hold. If you do decide to separate them, it seems to me that we should probably split the change to __private out from the introduction of implicit address spaces. Just let me know what you want to do. (*) I know that you aren't considering OpenCL C++ yet, but often these representation/model questions are easier to answer when thinking about C++ instead of C because type differences are so much more directly important in C++. In OpenCL C++, I assume it will be important to distinguish between and <__private int> as template arguments. That tells us straight up that __private needs to be represented differently from a lack of qualifier. Note that the result, where certain type representations (e.g. an unqualified int*) become basically meaningless and should never be constructed by Sema, is already precedented in Clang: ObjC ARC does the same thing to unqualified ObjC pointer types, like id*. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added a comment. In https://reviews.llvm.org/D35082#890162, @rjmccall wrote: > Okay. I think I see your point about why it would be better to have a > canonical __private address space that is different from the implicit address > space 0. I assume this means that there should basically never be pointers, > references, or l-values in address space 0 in OpenCL. If you mean 0 is `private` address space then no. There can be private AS pointers. But if you mean 0 is no address space then this doesn't exist in OpenCL. I think the right design in the first place would have been to keep address spaces in AST just as they are in the source code and add explicit address spaces to all types in IR instead. In this case absence of any address spaces in AST would signal implicit AS to be used. This would make some Sema checks a bit more complicated though, but the design would become a lot cleaner. Unfortunately I think it would not be worth doing this big change now. > You will lose a significant amount of representational efficiency by doing > this, but it's probably not overwhelming. > > I know you aren't implementing OpenCL C++ yet, so most of the cases where > temporaries are introduced aren't meaningful to you, but you will at least > need to consider compound literals. I suspect the right rule is that > file-scope literals should be inferred as being in __global or __constant > memory, depending on whether they're const, and local-scope literals should > be inferred as __private. > > I'll try to review your patch tomorrow. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment. Okay. I think I see your point about why it would be better to have a canonical __private address space that is different from the implicit address space 0. I assume this means that there should basically never be pointers, references, or l-values in address space 0 in OpenCL. You will lose a significant amount of representational efficiency by doing this, but it's probably not overwhelming. I know you aren't implementing OpenCL C++ yet, so most of the cases where temporaries are introduced aren't meaningful to you, but you will at least need to consider compound literals. I suspect the right rule is that file-scope literals should be inferred as being in __global or __constant memory, depending on whether they're const, and local-scope literals should be inferred as __private. I'll try to review your patch tomorrow. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl added a comment. In https://reviews.llvm.org/D35082#890150, @rjmccall wrote: > Non-canonical information is not supposed to be mangled. > > It's not clear to me what the language rule you're really trying to implement > is. Maybe you really do need a canonical __private address space, in which > case you are going to have to change a lot of code in Sema to add the address > space qualifier to every local and temporary allocation. But that's not > obvious to me yet, because you haven't really explained why it needs to be > explicit in the representation. In OpenCL all memory is in certain address space, therefore every l-value and pointee in a pointer should have an address space. Private address space has equal status as global or local address space. There are language rules about pointers to what address space can be assigned to pointers to what address space. Therefore all address space needs to be canonical. This patch already puts every local variable and function parameter in private address space, as is done in deduceOpenCLImplicitAddrSpace(). We need private address space to be explicit because we need to display explicit private address space and implicit address space differently, also because in certain semantic checks we need to know if a private address space is explicit or not. > If you just want pointers to __private to be mangled with an address-space > qualifier — meaning I guess that the mangling e.g. Pi will be completely > unused — that should be easy enough to handle in the mangler. But if you > need to distinguish between __private-qualified types and unqualified types, > and that distinction isn't purely to implement some syntactic restriction > about not writing e.g. __private __global, then that's not good enough and > you do need a canonical __private. The mangler does not mangle an empty type qualifier, therefore if private address space is represented as the default address space (i.e., no address space), it will not be mangled at all. This is also related to the substitution and cannot be easily changed without breaking lots of stuff in the mangler. > Telling me that you're in a hurry isn't helpful; preserving a reasonable > representation and not allowing corner cases to become maintenance problems > is far more important to the project than landing patches in trunk on some > particular schedule. The introduction of explicit private address space and the implicit address space flag in AST is precisely for handling those corner cases in the OpenCL language rules so that they won't become maintenance problems. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment. Non-canonical information is not supposed to be mangled. It's not clear to me what the language rule you're really trying to implement is. Maybe you really do need a canonical __private address space, in which case you are going to have to change a lot of code in Sema to add the address space qualifier to every local and temporary allocation. But that's not obvious to me yet, because you haven't really explained why it needs to be explicit in the representation. If you just want pointers to __private to be mangled with an address-space qualifier — meaning I guess that the mangling e.g. Pi will be completely unused — that should be easy enough to handle in the mangler. But if you need to distinguish between __private-qualified types and unqualified types, and that distinction isn't purely to implement some syntactic restriction about not writing e.g. __private __global, then that's not good enough and you do need a canonical __private. Telling me that you're in a hurry isn't helpful; preserving a reasonable representation and not allowing corner cases to become maintenance problems is far more important to the project than landing patches in trunk on some particular schedule. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl added a comment. In https://reviews.llvm.org/D35082#890143, @rjmccall wrote: > You have an important backend change relying on being able to preserve type > sugar better in diagnostics? The only apparent semantic change in this patch > is that you're changing the mangling, which frankly seems incorrect. Can you elaborate on why the mangling is incorrect? https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment. You have an important backend change relying on being able to preserve type sugar better in diagnostics? The only apparent semantic change in this patch is that you're changing the mangling, which frankly seems incorrect. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl added a comment. In https://reviews.llvm.org/D35082#889053, @rjmccall wrote: > Are you sure it's a good idea to not print the address space when it's > implicit? Won't that often lead to really confusing diagnostics? > > Also, we do already have a way of expressing that an extended qualifier was > explicit: AttributedType. We have very similar sorts of superficial > well-formedness checks to what I think you're trying to do in ObjC ARC. Based on my observation, in most cases it is OK not to print the implicit address space, and printing implicit address space could cause quite annoying cluttering. In some cases printing implicit address space may be desired. I can improve the printing in some future patch, e.g. only hide the implicit address space in situations which causes cluttering but not providing much useful information. I think AttributedType sounds an interesting idea and worth exploring. I just felt this review dragged too long (~ 3 months) already. We have an important backend change depending on this feature. Since the current solution achieves its goals already, can we leave re-implementing it by AttributedType for the future? Thanks. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment. Are you sure it's a good idea to not print the address space when it's implicit? Won't that often lead to really confusing diagnostics? Also, we do already have a way of expressing that an extended qualifier was explicit: AttributedType. We have very similar sorts of superficial well-formedness checks to what I think you're trying to do in ObjC ARC. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl added a comment.
In https://reviews.llvm.org/D35082#887855, @rjmccall wrote:
> Why is most of this patch necessary under the design of adding a
> non-canonical __private address space?
There are two reasons that we need a flag to indicate an address space is
simplicit:
1. We need a consistent way to print the address space qualifier depending on
whether it is implicit or not.
We only print address space qualifier when it is explicit. This is not just for
private address space. It is for all
address spaces.
2. In some rare situations we need to know whether an address space is implicit
when doing the semantic
checking.
Since the implicit property is per address space qualifier, we need this flag
to be on the qualifier.
Comment at: include/clang/AST/Type.h:336
+ /// space makes difference.
+ bool getImplicitAddressSpaceFlag() const { return Mask & IMask; }
+ void setImplicitAddressSpaceFlag(bool Value) {
rjmccall wrote:
> isAddressSpaceImplicit()
Will change.
Comment at: include/clang/AST/Type.h:337
+ bool getImplicitAddressSpaceFlag() const { return Mask & IMask; }
+ void setImplicitAddressSpaceFlag(bool Value) {
+Mask = (Mask & ~IMask) | (((uint32_t)Value) << IShift);
rjmccall wrote:
> setAddressSpaceImplicit
Will change.
Comment at: lib/AST/ItaniumMangle.cpp:2232
case LangAS::opencl_constant: ASString = "CLconstant"; break;
+ case LangAS::opencl_private: ASString = "CLprivate"; break;
case LangAS::opencl_generic: ASString = "CLgeneric"; break;
rjmccall wrote:
> In what situation is this mangled? I thought we agree this was non-canonical.
OpenCL has overloaded builtin functions, e.g. `__attribute__((overloadable))
void f(private int*)` and `__attribute__((overloadable)) void f(global int*)`.
These functions need to be mangled so that the mangled names are different.
Comment at: test/SemaOpenCL/storageclass.cl:63
+ static float l_implicit_static_var = 0; // expected-error
{{variables in function scope cannot be declared static}}
+ static constant float l_constant_static_var = 0; // expected-error
{{variables in function scope cannot be declared static}}
+ static global float l_global_static_var = 0; // expected-error
{{variables in function scope cannot be declared static}}
Anastasia wrote:
> Does it make sense to put different address spaces here since this code is
> rejected earlier anyway?
In Sema I saw code handling different address spaces in various places. I want
to make sure that all address spaces are handled correctly.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl updated this revision to Diff 117770.
yaxunl marked 9 inline comments as done.
yaxunl added a comment.
Revised by John's comments.
https://reviews.llvm.org/D35082
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
include/clang/Basic/AddressSpaces.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/TypePrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/Basic/Targets/NVPTX.h
lib/Basic/Targets/SPIR.h
lib/Basic/Targets/TCE.h
lib/CodeGen/CGDecl.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGen/blocks-opencl.cl
test/CodeGenOpenCL/address-spaces-mangling.cl
test/CodeGenOpenCL/address-spaces.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
test/SemaOpenCL/address-spaces.cl
test/SemaOpenCL/atomic-ops.cl
test/SemaOpenCL/cl20-device-side-enqueue.cl
test/SemaOpenCL/extern.cl
test/SemaOpenCL/invalid-block.cl
test/SemaOpenCL/invalid-pipes-cl2.0.cl
test/SemaOpenCL/null_literal.cl
test/SemaOpenCL/storageclass-cl20.cl
test/SemaOpenCL/storageclass.cl
Index: test/SemaOpenCL/storageclass.cl
===
--- test/SemaOpenCL/storageclass.cl
+++ test/SemaOpenCL/storageclass.cl
@@ -5,6 +5,20 @@
int G3 = 0;// expected-error{{program scope variable must reside in constant address space}}
global int G4 = 0; // expected-error{{program scope variable must reside in constant address space}}
+static float g_implicit_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static constant float g_constant_static_var = 0;
+static global float g_global_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static local float g_local_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static private float g_private_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static generic float g_generic_static_var = 0; // expected-error{{OpenCL version 1.2 does not support the 'generic' type qualifier}} // expected-error {{program scope variable must reside in constant address space}}
+
+extern float g_implicit_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern constant float g_constant_extern_var;
+extern global float g_global_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern local float g_local_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern private float g_private_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern generic float g_generic_extern_var; // expected-error{{OpenCL version 1.2 does not support the 'generic' type qualifier}} // expected-error {{extern variable must reside in constant address space}}
+
void kernel foo(int x) {
// static is not allowed at local scope before CL2.0
static int S1 = 5; // expected-error{{variables in function scope cannot be declared static}}
@@ -45,10 +59,17 @@
__attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
}
+ static float l_implicit_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static constant float l_constant_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static global float l_global_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static local float l_local_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static private float l_private_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static generic float l_generic_static_var = 0; // expected-error{{OpenCL version 1.2 does not support the 'generic' type qualifier}} // expected-error {{variables in function scope cannot be declared static}}
- extern constant float L5;
- extern local float L6; // expected-error{{extern variable must reside in constant address space}}
-
- static int L7 = 0; // expected-error{{variables in function scope cannot be declared static}}
- static int L8; // expected-error{{variables in function scope cannot be declared static}}
+ extern float l_implicit_extern_var; // expected-error {{extern variable must reside in constant address space}}
+ extern constant float l_constant_extern_var;
+ extern global float l_global_extern_var; // expected-error {{extern variable must reside in constant address space}}
+ extern local float l_local_extern_var; // expected-error {{extern variable must reside in constant address space}}
+ extern private float l_private_extern_var; //
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added inline comments.
Comment at: lib/Sema/SemaType.cpp:6872
+ ImpAddr = LangAS::opencl_private;
+else if (IsStatic)
+ ImpAddr = LangAS::opencl_global;
yaxunl wrote:
> Anastasia wrote:
> > I think we can't have this case for CL1.2 see s6.8. But I think it could
> > happen for `extern` though.
> Right. I will remove setting implicit addr space for static var for CL1.2.
>
> For extern var, for CL2.0 I will set implicit addr space to global.
>
> However, for CL1.2 since only constant addr space is supported for file-scope
> var, I can only set the implicit addr space of an extern var to be constant.
> However I feel this may cause more confusion than convenience, therefore I
> will not set implicit addr space for extern var for CL1.2. If user does not
> use constant addr space with extern var explicitly, they will see diagnostics
> that extern var must have constant addr space. This is also the current
> behavior before my change.
Makes sense!
Comment at: test/SemaOpenCL/storageclass.cl:63
+ static float l_implicit_static_var = 0; // expected-error
{{variables in function scope cannot be declared static}}
+ static constant float l_constant_static_var = 0; // expected-error
{{variables in function scope cannot be declared static}}
+ static global float l_global_static_var = 0; // expected-error
{{variables in function scope cannot be declared static}}
Does it make sense to put different address spaces here since this code is
rejected earlier anyway?
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment.
Why is most of this patch necessary under the design of adding a non-canonical
__private address space?
Comment at: include/clang/AST/Type.h:336
+ /// space makes difference.
+ bool getImplicitAddressSpaceFlag() const { return Mask & IMask; }
+ void setImplicitAddressSpaceFlag(bool Value) {
isAddressSpaceImplicit()
Comment at: include/clang/AST/Type.h:337
+ bool getImplicitAddressSpaceFlag() const { return Mask & IMask; }
+ void setImplicitAddressSpaceFlag(bool Value) {
+Mask = (Mask & ~IMask) | (((uint32_t)Value) << IShift);
setAddressSpaceImplicit
Comment at: lib/AST/ItaniumMangle.cpp:2232
case LangAS::opencl_constant: ASString = "CLconstant"; break;
+ case LangAS::opencl_private: ASString = "CLprivate"; break;
case LangAS::opencl_generic: ASString = "CLgeneric"; break;
In what situation is this mangled? I thought we agree this was non-canonical.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl updated this revision to Diff 117020.
yaxunl marked 3 inline comments as done.
yaxunl added a comment.
Revised by Anastasia's comments.
https://reviews.llvm.org/D35082
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
include/clang/Basic/AddressSpaces.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/TypePrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/Basic/Targets/NVPTX.h
lib/Basic/Targets/SPIR.h
lib/Basic/Targets/TCE.h
lib/CodeGen/CGDecl.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGen/blocks-opencl.cl
test/CodeGenOpenCL/address-spaces-mangling.cl
test/CodeGenOpenCL/address-spaces.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
test/SemaOpenCL/address-spaces.cl
test/SemaOpenCL/atomic-ops.cl
test/SemaOpenCL/cl20-device-side-enqueue.cl
test/SemaOpenCL/extern.cl
test/SemaOpenCL/invalid-block.cl
test/SemaOpenCL/invalid-pipes-cl2.0.cl
test/SemaOpenCL/null_literal.cl
test/SemaOpenCL/storageclass-cl20.cl
test/SemaOpenCL/storageclass.cl
Index: test/SemaOpenCL/storageclass.cl
===
--- test/SemaOpenCL/storageclass.cl
+++ test/SemaOpenCL/storageclass.cl
@@ -5,6 +5,20 @@
int G3 = 0;// expected-error{{program scope variable must reside in constant address space}}
global int G4 = 0; // expected-error{{program scope variable must reside in constant address space}}
+static float g_implicit_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static constant float g_constant_static_var = 0;
+static global float g_global_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static local float g_local_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static private float g_private_static_var = 0; // expected-error {{program scope variable must reside in constant address space}}
+static generic float g_generic_static_var = 0; // expected-error{{OpenCL version 1.2 does not support the 'generic' type qualifier}} // expected-error {{program scope variable must reside in constant address space}}
+
+extern float g_implicit_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern constant float g_constant_extern_var;
+extern global float g_global_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern local float g_local_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern private float g_private_extern_var; // expected-error {{extern variable must reside in constant address space}}
+extern generic float g_generic_extern_var; // expected-error{{OpenCL version 1.2 does not support the 'generic' type qualifier}} // expected-error {{extern variable must reside in constant address space}}
+
void kernel foo(int x) {
// static is not allowed at local scope before CL2.0
static int S1 = 5; // expected-error{{variables in function scope cannot be declared static}}
@@ -45,10 +59,17 @@
__attribute__((address_space(100))) int L4; // expected-error{{automatic variable qualified with an invalid address space}}
}
+ static float l_implicit_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static constant float l_constant_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static global float l_global_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static local float l_local_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static private float l_private_static_var = 0; // expected-error {{variables in function scope cannot be declared static}}
+ static generic float l_generic_static_var = 0; // expected-error{{OpenCL version 1.2 does not support the 'generic' type qualifier}} // expected-error {{variables in function scope cannot be declared static}}
- extern constant float L5;
- extern local float L6; // expected-error{{extern variable must reside in constant address space}}
-
- static int L7 = 0; // expected-error{{variables in function scope cannot be declared static}}
- static int L8; // expected-error{{variables in function scope cannot be declared static}}
+ extern float l_implicit_extern_var; // expected-error {{extern variable must reside in constant address space}}
+ extern constant float l_constant_extern_var;
+ extern global float l_global_extern_var; // expected-error {{extern variable must reside in constant address space}}
+ extern local float l_local_extern_var; // expected-error {{extern variable must reside in constant address space}}
+ extern private float l_private_extern_var; //
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl marked 5 inline comments as done.
yaxunl added inline comments.
Comment at: lib/Sema/SemaType.cpp:6810
+ QualType , TypeAttrLocation TAL) {
+ if (!State.getSema().getLangOpts().OpenCL ||
+ T.getAddressSpace() != LangAS::Default)
Anastasia wrote:
> I think this could be checked before calling the function.
will do.
Comment at: lib/Sema/SemaType.cpp:6863
+ unsigned ImpAddr;
+ bool IsStatic = D.getDeclSpec().getStorageClassSpec() ==
DeclSpec::SCS_static;
+ // Put OpenCL automatic variable in private address space.
Anastasia wrote:
> Do we cover `extern` too?
will add.
Comment at: lib/Sema/SemaType.cpp:6872
+ ImpAddr = LangAS::opencl_private;
+else if (IsStatic)
+ ImpAddr = LangAS::opencl_global;
Anastasia wrote:
> I think we can't have this case for CL1.2 see s6.8. But I think it could
> happen for `extern` though.
Right. I will remove setting implicit addr space for static var for CL1.2.
For extern var, for CL2.0 I will set implicit addr space to global.
However, for CL1.2 since only constant addr space is supported for file-scope
var, I can only set the implicit addr space of an extern var to be constant.
However I feel this may cause more confusion than convenience, therefore I will
not set implicit addr space for extern var for CL1.2. If user does not use
constant addr space with extern var explicitly, they will see diagnostics that
extern var must have constant addr space. This is also the current behavior
before my change.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added inline comments.
Comment at: lib/Sema/SemaType.cpp:6808
+static void deduceOpenCLImplicitAddrSpace(TypeProcessingState ,
+ QualType , TypeAttrLocation TAL) {
Great! This looks so clear now!
Comment at: lib/Sema/SemaType.cpp:6810
+ QualType , TypeAttrLocation TAL) {
+ if (!State.getSema().getLangOpts().OpenCL ||
+ T.getAddressSpace() != LangAS::Default)
I think this could be checked before calling the function.
Comment at: lib/Sema/SemaType.cpp:6863
+ unsigned ImpAddr;
+ bool IsStatic = D.getDeclSpec().getStorageClassSpec() ==
DeclSpec::SCS_static;
+ // Put OpenCL automatic variable in private address space.
Do we cover `extern` too?
Comment at: lib/Sema/SemaType.cpp:6872
+ ImpAddr = LangAS::opencl_private;
+else if (IsStatic)
+ ImpAddr = LangAS::opencl_global;
I think we can't have this case for CL1.2 see s6.8. But I think it could happen
for `extern` though.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl marked an inline comment as done. yaxunl added inline comments. Comment at: lib/Sema/SemaType.cpp:6974 + if (state.getSema().getLangOpts().OpenCL && !hasOpenCLAddressSpace && + type.getAddressSpace() == LangAS::Default && Anastasia wrote: > I am not very convinced we need to do all this check really... I think since > we have to live with this code further and maintain it let's try to simplify > it a bit. This can also help us avoid redundant checks. Perhaps we could even > move it out in a separate function? > > How about some sort of hierarchical check structure like: > > | |**CL version?** || > |<2.0||>=2.0| > |`private`||**Type?**|| > ||pointer||non-pointer| > ||`generic`||**Scope?**|| > |||program||function| > |||`global`||**Qualifiers?**| > none||static/extern| > `private`||`global`| > fixed. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl updated this revision to Diff 116704.
yaxunl marked 5 inline comments as done.
yaxunl added a comment.
Rebase to ToT and clean up logic.
https://reviews.llvm.org/D35082
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
include/clang/Basic/AddressSpaces.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/TypePrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/Basic/Targets/NVPTX.h
lib/Basic/Targets/SPIR.h
lib/Basic/Targets/TCE.h
lib/CodeGen/CGDecl.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGen/blocks-opencl.cl
test/CodeGenOpenCL/address-spaces-mangling.cl
test/CodeGenOpenCL/address-spaces.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
test/SemaOpenCL/address-spaces.cl
test/SemaOpenCL/atomic-ops.cl
test/SemaOpenCL/cl20-device-side-enqueue.cl
test/SemaOpenCL/invalid-block.cl
test/SemaOpenCL/invalid-pipes-cl2.0.cl
test/SemaOpenCL/null_literal.cl
Index: test/SemaOpenCL/null_literal.cl
===
--- test/SemaOpenCL/null_literal.cl
+++ test/SemaOpenCL/null_literal.cl
@@ -1,29 +1,68 @@
// RUN: %clang_cc1 -verify %s
-// RUN: %clang_cc1 -cl-std=CL2.0 -DCL20 -verify %s
+// RUN: %clang_cc1 -cl-std=CL2.0 -verify %s
#define NULL ((void*)0)
void foo(){
+ global int *g1 = NULL;
+ global int *g2 = (global void *)0;
+ global int *g3 = (constant void *)0; // expected-error{{initializing '__global int *' with an expression of type '__constant void *' changes address space of pointer}}
+ global int *g4 = (local void *)0; // expected-error{{initializing '__global int *' with an expression of type '__local void *' changes address space of pointer}}
+ global int *g5 = (private void *)0; // expected-error{{initializing '__global int *' with an expression of type '__private void *' changes address space of pointer}}
-global int* ptr1 = NULL;
+ constant int *c1 = NULL;
+ constant int *c2 = (global void *)0; // expected-error{{initializing '__constant int *' with an expression of type '__global void *' changes address space of pointer}}
+ constant int *c3 = (constant void *)0;
+ constant int *c4 = (local void *)0; // expected-error{{initializing '__constant int *' with an expression of type '__local void *' changes address space of pointer}}
+ constant int *c5 = (private void *)0; // expected-error{{initializing '__constant int *' with an expression of type '__private void *' changes address space of pointer}}
-global int* ptr2 = (global void*)0;
+ local int *l1 = NULL;
+ local int *l2 = (global void *)0; // expected-error{{initializing '__local int *' with an expression of type '__global void *' changes address space of pointer}}
+ local int *l3 = (constant void *)0; // expected-error{{initializing '__local int *' with an expression of type '__constant void *' changes address space of pointer}}
+ local int *l4 = (local void *)0;
+ local int *l5 = (private void *)0; // expected-error{{initializing '__local int *' with an expression of type '__private void *' changes address space of pointer}}
-constant int* ptr3 = NULL;
+ private int *p1 = NULL;
+ private int *p2 = (global void *)0; // expected-error{{initializing '__private int *' with an expression of type '__global void *' changes address space of pointer}}
+ private int *p3 = (constant void *)0; // expected-error{{initializing '__private int *' with an expression of type '__constant void *' changes address space of pointer}}
+ private int *p4 = (local void *)0; // expected-error{{initializing '__private int *' with an expression of type '__local void *' changes address space of pointer}}
+ private int *p5 = (private void *)0;
-constant int* ptr4 = (global void*)0; // expected-error{{initializing '__constant int *' with an expression of type '__global void *' changes address space of pointer}}
+#if __OPENCL_C_VERSION__ >= 200
+ // Assigning a pointer to a pointer to narrower address space causes an error unless there is an valid explicit cast.
+ global int *g6 = (generic void *)0; // expected-error{{initializing '__global int *' with an expression of type '__generic void *' changes address space of pointer}}
+ constant int *c6 = (generic void *)0; // expected-error{{initializing '__constant int *' with an expression of type '__generic void *' changes address space of pointer}}
+ local int *l6 = (generic void *)0; // expected-error{{initializing '__local int *' with an expression of type '__generic void *' changes address space of pointer}}
+ private int *p6 = (generic void *)0; // expected-error{{initializing '__private int *' with an expression of type '__generic void *' changes address space of pointer}}
-#ifdef CL20
-// Accept explicitly pointer to generic address space in OpenCL v2.0.
-global int* ptr5 = (generic void*)0;
-#endif
-
-global int* ptr6 = (local void*)0; // expected-error{{initializing '__global int *' with an expression of
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added inline comments. Comment at: lib/Sema/SemaType.cpp:6994 + // OpenCL v1.2 s6.5: + // The generic address space name for arguments to a function in a + // program, or local variables of a function is __private. All function yaxunl wrote: > Anastasia wrote: > > yaxunl wrote: > > > Anastasia wrote: > > > > The generic address space -> The default address space > > > 'The generic address space name for arguments' is literally from the > > > OpenCL 1.2 spec. Note it refers 'generic address space name', which is > > > not the 'generic address space' defined by OpenCL 2.0 spec. > > True, but this spec was written before v2.0 was released. And I feel now it > > makes things confusing considering that we have v2.0 implementation too. > I can make the change. I just feel a little bit uneasy since this looks like > a citation but actually is rephrased. Cool! Thanks! https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl marked 2 inline comments as done. yaxunl added inline comments. Comment at: lib/Sema/SemaType.cpp:6994 + // OpenCL v1.2 s6.5: + // The generic address space name for arguments to a function in a + // program, or local variables of a function is __private. All function Anastasia wrote: > yaxunl wrote: > > Anastasia wrote: > > > The generic address space -> The default address space > > 'The generic address space name for arguments' is literally from the OpenCL > > 1.2 spec. Note it refers 'generic address space name', which is not the > > 'generic address space' defined by OpenCL 2.0 spec. > True, but this spec was written before v2.0 was released. And I feel now it > makes things confusing considering that we have v2.0 implementation too. I can make the change. I just feel a little bit uneasy since this looks like a citation but actually is rephrased. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added inline comments. Comment at: lib/Sema/SemaType.cpp:6994 + // OpenCL v1.2 s6.5: + // The generic address space name for arguments to a function in a + // program, or local variables of a function is __private. All function yaxunl wrote: > Anastasia wrote: > > The generic address space -> The default address space > 'The generic address space name for arguments' is literally from the OpenCL > 1.2 spec. Note it refers 'generic address space name', which is not the > 'generic address space' defined by OpenCL 2.0 spec. True, but this spec was written before v2.0 was released. And I feel now it makes things confusing considering that we have v2.0 implementation too. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl updated this revision to Diff 114337.
yaxunl marked 3 inline comments as done.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Add comments for getImplicitAddressSpaceFlag and fix checking of null pointer.
https://reviews.llvm.org/D35082
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
include/clang/Basic/AddressSpaces.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/TypePrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/Basic/Targets/NVPTX.h
lib/Basic/Targets/SPIR.h
lib/Basic/Targets/TCE.h
lib/CodeGen/CGDecl.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCL/address-spaces-mangling.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
test/SemaOpenCL/address-spaces.cl
test/SemaOpenCL/atomic-ops.cl
test/SemaOpenCL/cl20-device-side-enqueue.cl
test/SemaOpenCL/invalid-block.cl
test/SemaOpenCL/invalid-pipes-cl2.0.cl
test/SemaOpenCL/null_literal.cl
Index: test/SemaOpenCL/null_literal.cl
===
--- test/SemaOpenCL/null_literal.cl
+++ test/SemaOpenCL/null_literal.cl
@@ -1,29 +1,68 @@
// RUN: %clang_cc1 -verify %s
-// RUN: %clang_cc1 -cl-std=CL2.0 -DCL20 -verify %s
+// RUN: %clang_cc1 -cl-std=CL2.0 -verify %s
#define NULL ((void*)0)
void foo(){
+ global int *g1 = NULL;
+ global int *g2 = (global void *)0;
+ global int *g3 = (constant void *)0; // expected-error{{initializing '__global int *' with an expression of type '__constant void *' changes address space of pointer}}
+ global int *g4 = (local void *)0; // expected-error{{initializing '__global int *' with an expression of type '__local void *' changes address space of pointer}}
+ global int *g5 = (private void *)0; // expected-error{{initializing '__global int *' with an expression of type '__private void *' changes address space of pointer}}
-global int* ptr1 = NULL;
+ constant int *c1 = NULL;
+ constant int *c2 = (global void *)0; // expected-error{{initializing '__constant int *' with an expression of type '__global void *' changes address space of pointer}}
+ constant int *c3 = (constant void *)0;
+ constant int *c4 = (local void *)0; // expected-error{{initializing '__constant int *' with an expression of type '__local void *' changes address space of pointer}}
+ constant int *c5 = (private void *)0; // expected-error{{initializing '__constant int *' with an expression of type '__private void *' changes address space of pointer}}
-global int* ptr2 = (global void*)0;
+ local int *l1 = NULL;
+ local int *l2 = (global void *)0; // expected-error{{initializing '__local int *' with an expression of type '__global void *' changes address space of pointer}}
+ local int *l3 = (constant void *)0; // expected-error{{initializing '__local int *' with an expression of type '__constant void *' changes address space of pointer}}
+ local int *l4 = (local void *)0;
+ local int *l5 = (private void *)0; // expected-error{{initializing '__local int *' with an expression of type '__private void *' changes address space of pointer}}
-constant int* ptr3 = NULL;
+ private int *p1 = NULL;
+ private int *p2 = (global void *)0; // expected-error{{initializing '__private int *' with an expression of type '__global void *' changes address space of pointer}}
+ private int *p3 = (constant void *)0; // expected-error{{initializing '__private int *' with an expression of type '__constant void *' changes address space of pointer}}
+ private int *p4 = (local void *)0; // expected-error{{initializing '__private int *' with an expression of type '__local void *' changes address space of pointer}}
+ private int *p5 = (private void *)0;
-constant int* ptr4 = (global void*)0; // expected-error{{initializing '__constant int *' with an expression of type '__global void *' changes address space of pointer}}
+#if __OPENCL_C_VERSION__ >= 200
+ // Assigning a pointer to a pointer to narrower address space causes an error unless there is an valid explicit cast.
+ global int *g6 = (generic void *)0; // expected-error{{initializing '__global int *' with an expression of type '__generic void *' changes address space of pointer}}
+ constant int *c6 = (generic void *)0; // expected-error{{initializing '__constant int *' with an expression of type '__generic void *' changes address space of pointer}}
+ local int *l6 = (generic void *)0; // expected-error{{initializing '__local int *' with an expression of type '__generic void *' changes address space of pointer}}
+ private int *p6 = (generic void *)0; // expected-error{{initializing '__private int *' with an expression of type '__generic void *' changes address space of pointer}}
-#ifdef CL20
-// Accept explicitly pointer to generic address space in OpenCL v2.0.
-global int* ptr5 = (generic void*)0;
-#endif
-
-global int* ptr6 = (local void*)0; // expected-error{{initializing '__global int *' with an
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl marked 6 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/AST/Type.h:332
+ bool getImplicitAddressSpaceFlag() const { return Mask & IMask; }
+ void setImplicitAddressSpaceFlag(bool Value) {
Anastasia wrote:
> Could we add a bit of comment somewhere explaining why we added implicit AS
> flag please.
Will do.
Comment at: lib/AST/Expr.cpp:3254
+// Only (void*)0 or equivalent are treated as nullptr. If pointee type
+// has non-default address space it is not treated as nullptr.
+bool PointeeHasDefaultAS = false;
Anastasia wrote:
> Is this comment right wrt to named address spaces. So is `(private void*)0`
> not a `nullptr`?
Yes this will be true.
Neither (private void*)0 nor (__generic void*)0 should be treated nullptr since
they cannot be assigned to a constant pointer.
This need some change below to check implicity of the address space. I will do
that. Also add more tests about this.
Comment at: lib/Sema/SemaType.cpp:6994
+ // OpenCL v1.2 s6.5:
+ // The generic address space name for arguments to a function in a
+ // program, or local variables of a function is __private. All function
Anastasia wrote:
> The generic address space -> The default address space
'The generic address space name for arguments' is literally from the OpenCL 1.2
spec. Note it refers 'generic address space name', which is not the 'generic
address space' defined by OpenCL 2.0 spec.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added inline comments.
Comment at: include/clang/AST/Type.h:332
+ bool getImplicitAddressSpaceFlag() const { return Mask & IMask; }
+ void setImplicitAddressSpaceFlag(bool Value) {
Could we add a bit of comment somewhere explaining why we added implicit AS
flag please.
Comment at: lib/AST/Expr.cpp:3254
+// Only (void*)0 or equivalent are treated as nullptr. If pointee type
+// has non-default address space it is not treated as nullptr.
+bool PointeeHasDefaultAS = false;
Is this comment right wrt to named address spaces. So is `(private void*)0` not
a `nullptr`?
Comment at: lib/AST/Expr.cpp:3257
+if (Ctx.getLangOpts().OpenCL)
+ PointeeHasDefaultAS =
+ (Ctx.getLangOpts().OpenCLVersion >= 200 &&
Can we now simplify this by checking implicit AS bit instead?
Comment at: lib/Sema/SemaType.cpp:6974
+ if (state.getSema().getLangOpts().OpenCL && !hasOpenCLAddressSpace &&
+ type.getAddressSpace() == LangAS::Default &&
I am not very convinced we need to do all this check really... I think since we
have to live with this code further and maintain it let's try to simplify it a
bit. This can also help us avoid redundant checks. Perhaps we could even move
it out in a separate function?
How about some sort of hierarchical check structure like:
| |**CL version?** ||
|<2.0||>=2.0|
|`private`||**Type?**||
||pointer||non-pointer|
||`generic`||**Scope?**||
|||program||function|
|||`global`||**Qualifiers?**|
none||static/extern|
`private`||`global`|
Comment at: lib/Sema/SemaType.cpp:6994
+ // OpenCL v1.2 s6.5:
+ // The generic address space name for arguments to a function in a
+ // program, or local variables of a function is __private. All function
The generic address space -> The default address space
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl marked an inline comment as done. yaxunl added a comment. ping https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:11846
// an address space.
if (T.getAddressSpace() != 0) {
// OpenCL allows function arguments declared to be an array of a type
yaxunl wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > Anastasia wrote:
> > > > yaxunl wrote:
> > > > > Anastasia wrote:
> > > > > > Could we use `LangAS::Default` here too?
> > > > > done
> > > > Sorry, I wasn't clear. I think we could have:
> > > >
> > > > if (T.getAddressSpace() != LangAS::Default && T.getAddressSpace() !=
> > > > LangAS::opencl_private)
> > > >
> > > > and then original condition. It is a bit clearer I think.
> > > No. For OpenCL, the condition is on line 11847 and 11848. An array type
> > > in other address spaces is allowed.
> > I think the initial condition was `T.getAddressSpace() != 0` i.e. if not
> > private address space.
> >
> > So now since we added private this should be `T.getAddressSpace()
> > !=LangAS::opencl_private` but we can have the default case as well hence
> > 'T.getAddressSpace() !=LangAS::opencl_private || T.getAddressSpace()
> > !=LangAS::Default'.
> >
> > This entire case seems to be for OpenCL anyways. So you could also move out
> > the OpenCL check if you prefer. I am just trying to see if we can make this
> > easier to understand.
> This is not just for OpenCL.
>
> The condition expresses the following logic:
>
> For non-OpenCL languages, only default addr space is allowed on function
> argument.
>
> For OpenCL, for array type argument, any addr space is allowed; for non-array
> type argument, only private addr space is allowed.
Parameters declared with type 'foo T[]' will be canonicalized to 'foo T*' when
forming the function type, so that's not actually a different rule in the end.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl marked 6 inline comments as done.
yaxunl added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:11846
// an address space.
if (T.getAddressSpace() != 0) {
// OpenCL allows function arguments declared to be an array of a type
Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > yaxunl wrote:
> > > > Anastasia wrote:
> > > > > Could we use `LangAS::Default` here too?
> > > > done
> > > Sorry, I wasn't clear. I think we could have:
> > >
> > > if (T.getAddressSpace() != LangAS::Default && T.getAddressSpace() !=
> > > LangAS::opencl_private)
> > >
> > > and then original condition. It is a bit clearer I think.
> > No. For OpenCL, the condition is on line 11847 and 11848. An array type in
> > other address spaces is allowed.
> I think the initial condition was `T.getAddressSpace() != 0` i.e. if not
> private address space.
>
> So now since we added private this should be `T.getAddressSpace()
> !=LangAS::opencl_private` but we can have the default case as well hence
> 'T.getAddressSpace() !=LangAS::opencl_private || T.getAddressSpace()
> !=LangAS::Default'.
>
> This entire case seems to be for OpenCL anyways. So you could also move out
> the OpenCL check if you prefer. I am just trying to see if we can make this
> easier to understand.
This is not just for OpenCL.
The condition expresses the following logic:
For non-OpenCL languages, only default addr space is allowed on function
argument.
For OpenCL, for array type argument, any addr space is allowed; for non-array
type argument, only private addr space is allowed.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl updated this revision to Diff 112288.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Herald added subscribers: nhaehnle, jholewinski.
Add a flag to indicate whether address space qualifier is implicit and only
print explicit address space in diagnostics.
https://reviews.llvm.org/D35082
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Type.h
include/clang/Basic/AddressSpaces.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/TypePrinter.cpp
lib/Basic/Targets/AMDGPU.cpp
lib/Basic/Targets/NVPTX.h
lib/Basic/Targets/SPIR.h
lib/Basic/Targets/TCE.h
lib/CodeGen/CGDecl.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCL/address-spaces-mangling.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
test/SemaOpenCL/address-spaces.cl
test/SemaOpenCL/atomic-ops.cl
test/SemaOpenCL/cl20-device-side-enqueue.cl
test/SemaOpenCL/invalid-block.cl
test/SemaOpenCL/invalid-pipes-cl2.0.cl
test/SemaOpenCL/null_literal.cl
Index: test/SemaOpenCL/null_literal.cl
===
--- test/SemaOpenCL/null_literal.cl
+++ test/SemaOpenCL/null_literal.cl
@@ -1,5 +1,5 @@
// RUN: %clang_cc1 -verify %s
-// RUN: %clang_cc1 -cl-std=CL2.0 -DCL20 -verify %s
+// RUN: %clang_cc1 -cl-std=CL2.0 -verify %s
#define NULL ((void*)0)
@@ -13,9 +13,17 @@
constant int* ptr4 = (global void*)0; // expected-error{{initializing '__constant int *' with an expression of type '__global void *' changes address space of pointer}}
-#ifdef CL20
+#if __OPENCL_C_VERSION__ >= 200
+
// Accept explicitly pointer to generic address space in OpenCL v2.0.
global int* ptr5 = (generic void*)0;
+
+global int *ptr = (private void *)0; // expected-error{{initializing '__global int *' with an expression of type '__private void *' changes address space of pointer}}
+
+#else
+
+global int *ptr = (private void *)0;
+
#endif
global int* ptr6 = (local void*)0; // expected-error{{initializing '__global int *' with an expression of type '__local void *' changes address space of pointer}}
Index: test/SemaOpenCL/invalid-pipes-cl2.0.cl
===
--- test/SemaOpenCL/invalid-pipes-cl2.0.cl
+++ test/SemaOpenCL/invalid-pipes-cl2.0.cl
@@ -3,7 +3,7 @@
global pipe int gp;// expected-error {{type '__global read_only pipe int' can only be used as a function parameter in OpenCL}}
global reserve_id_t rid; // expected-error {{the '__global reserve_id_t' type cannot be used to declare a program scope variable}}
-extern pipe write_only int get_pipe(); // expected-error {{type '__global write_only pipe int ()' can only be used as a function parameter in OpenCL}}
+extern pipe write_only int get_pipe(); // expected-error {{type 'write_only pipe int ()' can only be used as a function parameter in OpenCL}}
kernel void test_invalid_reserved_id(reserve_id_t ID) { // expected-error {{'reserve_id_t' cannot be used as the type of a kernel parameter}}
}
Index: test/SemaOpenCL/invalid-block.cl
===
--- test/SemaOpenCL/invalid-block.cl
+++ test/SemaOpenCL/invalid-block.cl
@@ -12,7 +12,7 @@
};
f0(bl1);
f0(bl2);
- bl1 = bl2; // expected-error{{invalid operands to binary expression ('int (__generic ^const)(void)' and 'int (__generic ^const)(void)')}}
+ bl1 = bl2; // expected-error{{invalid operands to binary expression ('int (^const)(void)' and 'int (^const)(void)')}}
int (^const bl3)(); // expected-error{{invalid block variable declaration - must be initialized}}
}
@@ -28,10 +28,10 @@
// A block cannot be the return value of a function.
typedef int (^bl_t)(void);
-bl_t f3(bl_t bl); // expected-error{{declaring function return value of type 'bl_t' (aka 'int (__generic ^const)(void)') is not allowed}}
+bl_t f3(bl_t bl); // expected-error{{declaring function return value of type 'bl_t' (aka 'int (^const)(void)') is not allowed}}
struct bl_s {
- int (^bl)(void); // expected-error {{the 'int (__generic ^const)(void)' type cannot be used to declare a structure or union field}}
+ int (^bl)(void); // expected-error {{the 'int (^const)(void)' type cannot be used to declare a structure or union field}}
};
void f4() {
@@ -53,18 +53,18 @@
bl2_t bl2 = ^(int i) {
return 2;
};
- bl2_t arr[] = {bl1, bl2}; // expected-error {{array of 'bl2_t' (aka 'int (__generic ^const)(int)') type is invalid in OpenCL}}
+ bl2_t arr[] = {bl1, bl2}; // expected-error {{array of 'bl2_t' (aka 'int (^const)(int)') type is invalid in OpenCL}}
int tmp = i ? bl1(i) // expected-error {{block type cannot be used as expression in ternary expression in OpenCL}}
: bl2(i); // expected-error {{block type cannot be used as expression in ternary expression in OpenCL}}
}
// A block pointer type and all pointer
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment. Well, we don't currently have a concept of a non-canonical qualifier, but I think it probably wouldn't be difficult to support; you would just need ASTContext::getQualifiedType to be aware of it. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl added a comment. In https://reviews.llvm.org/D35082#844620, @rjmccall wrote: > The meaning we've agreed on for LangAS::Default is to be the address space of > local declarations, which corresponds quite well to __private in OpenCL. I > think your concern about diagnostics is better addressed by changing the > pretty-printer than by changing Sema to give all local declarations qualified > type. How about adding a ImplicitAddrSpace bit to Qualifier to indicate address space is implicit, then does not print the address space qualifier in AST printer if the bit is set. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
rjmccall added a comment. The meaning we've agreed on for LangAS::Default is to be the address space of local declarations, which corresponds quite well to __private in OpenCL. I think your concern about diagnostics is better addressed by changing the pretty-printer than by changing Sema to give all local declarations qualified type. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl marked 2 inline comments as done. yaxunl added inline comments. Comment at: lib/Sema/SemaType.cpp:6969 + if (state.getSema().getLangOpts().OpenCL && + !hasOpenCLAddressSpace && type.getAddressSpace() == 0 && Anastasia wrote: > yaxunl wrote: > > Anastasia wrote: > > > yaxunl wrote: > > > > Anastasia wrote: > > > > > Would it be nicer to not append any address space at all neither here > > > > > nor down at the end of this function and keep it default instead > > > > > until the Codegen? If it's doable I would very much prefer that. It > > > > > seems like it would make the implementation potentially a bit cleaner > > > > > to understand and easier to improve semantical analysis. See one > > > > > example of improving original type printing in my comment to the test > > > > > below. > > > > > > > > > > Also there are at least these 3 related bugs open currently: > > > > > https://bugs.llvm.org//show_bug.cgi?id=33418 > > > > > https://bugs.llvm.org//show_bug.cgi?id=33419 > > > > > https://bugs.llvm.org//show_bug.cgi?id=33420 > > > > > > > > > > Does your change address any of those? > > > > On the contrary, I think using default address space for automatic > > > > variable and function parameter will cause ambiguity and inconsistency > > > > in AST, making it more difficult to understand and process, and making > > > > some bug (e.g. https://bugs.llvm.org//show_bug.cgi?id=33419) unfixable. > > > > For example, `private int f(void)` and `int f(void)` will be identical > > > > in AST, therefore we cannot diagnose `private int f(void)`. > > > > > > > > With current representation I am able to fix the 3 bugs. I will update > > > > the diff. > > > I don't see why? > > > > > > `private int f(void)` -> will have an address space attribute in AST as > > > it is provided explicitly. > > > > > > `int f(void) ` -> will have no address space attribute because it's not > > > provided explicitly and not attached implicitly either. > > > > > > All I was asking is not to deduce the address space here if it's not > > > specified explicitly until later step when we need to put it in the IR. > > Clang already deduce global and generic address spaces and use them in the > > diagnostic messages. I don't see why we can use deduced global and generic > > address space in diagnostics whereas cannot use deduced private address > > space in diagnostics. Why users can accept deduced global and generic > > address spaces but cannot accept deduced private address space? > > > > Automatic variables and function parameters have private address space. > > This is the reality and as true as a global variable has global or constant > > address spaces. Not using private address space in diagnostics gives user > > illusion that automatic variables and function parameters do not have > > address space, which is not true. > > > > Besides, allowing default address space to represent private address space > > in AST causes ambiguity in AST. Instead of just check if a type has private > > address space, now we need to check if a type has private or default > > address spaces. Also if an expression has default address space, it is not > > clear if it is an l-value or r-value. This will complicate semantic > > checking unnecessarily. Also I am not sure if it is acceptable to modify > > AST between Sema and CodeGen since it seems to change the paradigm of how > > clang does Sema/CodeGen now. > > Clang already deduce global and generic address spaces and use them in the > > diagnostic messages. I don't see why we can use deduced global and generic > > address space in diagnostics whereas cannot use deduced private address > > space in diagnostics. Why users can accept deduced global and generic > > address spaces but cannot accept deduced private address space? > > Yes, we did this initially as a workaround because there was no way to > distinguish the private and default address space. So now since we are adding > the private AS explicitly, it would be nice to remove the workaround if > that's possible. It's just a messy code which is hard to understand and we > had places in which we need to know if the address space was specified > explicitly or now. NULL pointer is one of them. There were also bugs > associated to this because it wasn't expected that the address space is > 'magically' attached during parsing. Even though most of the things are > solved now... I would prefer not to deduce any attributes at this place not > just private but global and default as well... > > > Besides, allowing default address space to represent private address space > > in AST causes ambiguity in AST. Instead of just check if a type has private > > address space, now we need to check if a type has private or default > > address spaces. > > I don't see any ambiguity, this is a difference in OpenCL wrt C that we have > implicit address space which we have to
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:11846
// an address space.
if (T.getAddressSpace() != 0) {
// OpenCL allows function arguments declared to be an array of a type
yaxunl wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > Anastasia wrote:
> > > > Could we use `LangAS::Default` here too?
> > > done
> > Sorry, I wasn't clear. I think we could have:
> >
> > if (T.getAddressSpace() != LangAS::Default && T.getAddressSpace() !=
> > LangAS::opencl_private)
> >
> > and then original condition. It is a bit clearer I think.
> No. For OpenCL, the condition is on line 11847 and 11848. An array type in
> other address spaces is allowed.
I think the initial condition was `T.getAddressSpace() != 0` i.e. if not
private address space.
So now since we added private this should be `T.getAddressSpace()
!=LangAS::opencl_private` but we can have the default case as well hence
'T.getAddressSpace() !=LangAS::opencl_private || T.getAddressSpace()
!=LangAS::Default'.
This entire case seems to be for OpenCL anyways. So you could also move out the
OpenCL check if you prefer. I am just trying to see if we can make this easier
to understand.
Comment at: lib/Sema/SemaType.cpp:6969
+ if (state.getSema().getLangOpts().OpenCL &&
+ !hasOpenCLAddressSpace && type.getAddressSpace() == 0 &&
yaxunl wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > Anastasia wrote:
> > > > Would it be nicer to not append any address space at all neither here
> > > > nor down at the end of this function and keep it default instead until
> > > > the Codegen? If it's doable I would very much prefer that. It seems
> > > > like it would make the implementation potentially a bit cleaner to
> > > > understand and easier to improve semantical analysis. See one example
> > > > of improving original type printing in my comment to the test below.
> > > >
> > > > Also there are at least these 3 related bugs open currently:
> > > > https://bugs.llvm.org//show_bug.cgi?id=33418
> > > > https://bugs.llvm.org//show_bug.cgi?id=33419
> > > > https://bugs.llvm.org//show_bug.cgi?id=33420
> > > >
> > > > Does your change address any of those?
> > > On the contrary, I think using default address space for automatic
> > > variable and function parameter will cause ambiguity and inconsistency in
> > > AST, making it more difficult to understand and process, and making some
> > > bug (e.g. https://bugs.llvm.org//show_bug.cgi?id=33419) unfixable. For
> > > example, `private int f(void)` and `int f(void)` will be identical in
> > > AST, therefore we cannot diagnose `private int f(void)`.
> > >
> > > With current representation I am able to fix the 3 bugs. I will update
> > > the diff.
> > I don't see why?
> >
> > `private int f(void)` -> will have an address space attribute in AST as it
> > is provided explicitly.
> >
> > `int f(void) ` -> will have no address space attribute because it's not
> > provided explicitly and not attached implicitly either.
> >
> > All I was asking is not to deduce the address space here if it's not
> > specified explicitly until later step when we need to put it in the IR.
> Clang already deduce global and generic address spaces and use them in the
> diagnostic messages. I don't see why we can use deduced global and generic
> address space in diagnostics whereas cannot use deduced private address space
> in diagnostics. Why users can accept deduced global and generic address
> spaces but cannot accept deduced private address space?
>
> Automatic variables and function parameters have private address space. This
> is the reality and as true as a global variable has global or constant
> address spaces. Not using private address space in diagnostics gives user
> illusion that automatic variables and function parameters do not have address
> space, which is not true.
>
> Besides, allowing default address space to represent private address space in
> AST causes ambiguity in AST. Instead of just check if a type has private
> address space, now we need to check if a type has private or default address
> spaces. Also if an expression has default address space, it is not clear if
> it is an l-value or r-value. This will complicate semantic checking
> unnecessarily. Also I am not sure if it is acceptable to modify AST between
> Sema and CodeGen since it seems to change the paradigm of how clang does
> Sema/CodeGen now.
> Clang already deduce global and generic address spaces and use them in the
> diagnostic messages. I don't see why we can use deduced global and generic
> address space in diagnostics whereas cannot use deduced private address space
> in diagnostics. Why users can accept deduced global and generic address
> spaces but cannot accept deduced private address space?
Yes, we did this initially as a workaround because there was no way to
distinguish the private and default address
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl marked 6 inline comments as done.
yaxunl added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:11846
// an address space.
if (T.getAddressSpace() != 0) {
// OpenCL allows function arguments declared to be an array of a type
Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > Could we use `LangAS::Default` here too?
> > done
> Sorry, I wasn't clear. I think we could have:
>
> if (T.getAddressSpace() != LangAS::Default && T.getAddressSpace() !=
> LangAS::opencl_private)
>
> and then original condition. It is a bit clearer I think.
No. For OpenCL, the condition is on line 11847 and 11848. An array type in
other address spaces is allowed.
Comment at: lib/Sema/SemaType.cpp:6969
+ if (state.getSema().getLangOpts().OpenCL &&
+ !hasOpenCLAddressSpace && type.getAddressSpace() == 0 &&
Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > Would it be nicer to not append any address space at all neither here nor
> > > down at the end of this function and keep it default instead until the
> > > Codegen? If it's doable I would very much prefer that. It seems like it
> > > would make the implementation potentially a bit cleaner to understand and
> > > easier to improve semantical analysis. See one example of improving
> > > original type printing in my comment to the test below.
> > >
> > > Also there are at least these 3 related bugs open currently:
> > > https://bugs.llvm.org//show_bug.cgi?id=33418
> > > https://bugs.llvm.org//show_bug.cgi?id=33419
> > > https://bugs.llvm.org//show_bug.cgi?id=33420
> > >
> > > Does your change address any of those?
> > On the contrary, I think using default address space for automatic variable
> > and function parameter will cause ambiguity and inconsistency in AST,
> > making it more difficult to understand and process, and making some bug
> > (e.g. https://bugs.llvm.org//show_bug.cgi?id=33419) unfixable. For example,
> > `private int f(void)` and `int f(void)` will be identical in AST, therefore
> > we cannot diagnose `private int f(void)`.
> >
> > With current representation I am able to fix the 3 bugs. I will update the
> > diff.
> I don't see why?
>
> `private int f(void)` -> will have an address space attribute in AST as it is
> provided explicitly.
>
> `int f(void) ` -> will have no address space attribute because it's not
> provided explicitly and not attached implicitly either.
>
> All I was asking is not to deduce the address space here if it's not
> specified explicitly until later step when we need to put it in the IR.
Clang already deduce global and generic address spaces and use them in the
diagnostic messages. I don't see why we can use deduced global and generic
address space in diagnostics whereas cannot use deduced private address space
in diagnostics. Why users can accept deduced global and generic address spaces
but cannot accept deduced private address space?
Automatic variables and function parameters have private address space. This is
the reality and as true as a global variable has global or constant address
spaces. Not using private address space in diagnostics gives user illusion that
automatic variables and function parameters do not have address space, which is
not true.
Besides, allowing default address space to represent private address space in
AST causes ambiguity in AST. Instead of just check if a type has private
address space, now we need to check if a type has private or default address
spaces. Also if an expression has default address space, it is not clear if it
is an l-value or r-value. This will complicate semantic checking unnecessarily.
Also I am not sure if it is acceptable to modify AST between Sema and CodeGen
since it seems to change the paradigm of how clang does Sema/CodeGen now.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added inline comments.
Comment at: lib/Sema/SemaDecl.cpp:11846
// an address space.
if (T.getAddressSpace() != 0) {
// OpenCL allows function arguments declared to be an array of a type
yaxunl wrote:
> Anastasia wrote:
> > Could we use `LangAS::Default` here too?
> done
Sorry, I wasn't clear. I think we could have:
if (T.getAddressSpace() != LangAS::Default && T.getAddressSpace() !=
LangAS::opencl_private)
and then original condition. It is a bit clearer I think.
Comment at: lib/Sema/SemaType.cpp:6969
+ if (state.getSema().getLangOpts().OpenCL &&
+ !hasOpenCLAddressSpace && type.getAddressSpace() == 0 &&
yaxunl wrote:
> Anastasia wrote:
> > Would it be nicer to not append any address space at all neither here nor
> > down at the end of this function and keep it default instead until the
> > Codegen? If it's doable I would very much prefer that. It seems like it
> > would make the implementation potentially a bit cleaner to understand and
> > easier to improve semantical analysis. See one example of improving
> > original type printing in my comment to the test below.
> >
> > Also there are at least these 3 related bugs open currently:
> > https://bugs.llvm.org//show_bug.cgi?id=33418
> > https://bugs.llvm.org//show_bug.cgi?id=33419
> > https://bugs.llvm.org//show_bug.cgi?id=33420
> >
> > Does your change address any of those?
> On the contrary, I think using default address space for automatic variable
> and function parameter will cause ambiguity and inconsistency in AST, making
> it more difficult to understand and process, and making some bug (e.g.
> https://bugs.llvm.org//show_bug.cgi?id=33419) unfixable. For example,
> `private int f(void)` and `int f(void)` will be identical in AST, therefore
> we cannot diagnose `private int f(void)`.
>
> With current representation I am able to fix the 3 bugs. I will update the
> diff.
I don't see why?
`private int f(void)` -> will have an address space attribute in AST as it is
provided explicitly.
`int f(void) ` -> will have no address space attribute because it's not
provided explicitly and not attached implicitly either.
All I was asking is not to deduce the address space here if it's not specified
explicitly until later step when we need to put it in the IR.
Comment at: test/SemaOpenCL/access-qualifier.cl:23
#else
-void myReadWrite(read_write image1d_t); // expected-error {{access qualifier
'read_write' can not be used for '__read_write image1d_t' prior to OpenCL
version 2.0}}
+void myReadWrite(read_write image1d_t); // expected-error {{access qualifier
'read_write' can not be used for '__private __read_write image1d_t' prior to
OpenCL version 2.0}}
#endif
yaxunl wrote:
> Anastasia wrote:
> > Ok, I think that here it would be less confusing not to add any address
> > space since it's missing in the original source.
> Although it looks strange at first sight, `__private __read_write image1d_t`
> is the true type of the argument. The function argument is allocated from
> stack and is an l-value. If we take its address, we get a private pointer.
Yes, it is a true type in the Clang internal representation indeed, but not in
the original source though. Here we are giving the feedback about the source
code so it's nicer to keep it as close to the original as possible. But we are
doing similar "magic" for blocks, images and other places, so it's not that
critical at the end. Just if it can be avoided it would be better.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl updated this revision to Diff 106486.
yaxunl marked 6 inline comments as done.
yaxunl added a comment.
Revised by Anastasia's comments.
https://reviews.llvm.org/D35082
Files:
include/clang/Basic/AddressSpaces.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/TypePrinter.cpp
lib/Basic/Targets.cpp
lib/CodeGen/CGDecl.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCL/address-spaces-mangling.cl
test/Index/opencl-types.cl
test/Parser/opencl-astype.cl
test/Parser/opencl-atomics-cl20.cl
test/SemaOpenCL/access-qualifier.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
test/SemaOpenCL/address-spaces.cl
test/SemaOpenCL/arithmetic-conversions.cl
test/SemaOpenCL/as_type.cl
test/SemaOpenCL/cl20-device-side-enqueue.cl
test/SemaOpenCL/event_t.cl
test/SemaOpenCL/extension-begin.cl
test/SemaOpenCL/half.cl
test/SemaOpenCL/images.cl
test/SemaOpenCL/invalid-block.cl
test/SemaOpenCL/invalid-image.cl
test/SemaOpenCL/invalid-kernel-parameters.cl
test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
test/SemaOpenCL/invalid-pipes-cl2.0.cl
test/SemaOpenCL/null_literal.cl
test/SemaOpenCL/null_queue.cl
test/SemaOpenCL/queue_t_overload.cl
test/SemaOpenCL/sampler_t.cl
test/SemaOpenCL/shifts.cl
test/SemaOpenCL/to_addr_builtin.cl
test/SemaOpenCL/vec_step.cl
test/SemaOpenCL/vector_conv_invalid.cl
Index: test/SemaOpenCL/vector_conv_invalid.cl
===
--- test/SemaOpenCL/vector_conv_invalid.cl
+++ test/SemaOpenCL/vector_conv_invalid.cl
@@ -7,7 +7,7 @@
void vector_conv_invalid() {
uint4 u = (uint4)(1);
- int4 i = u; // expected-error{{initializing 'int4' (vector of 4 'int' values) with an expression of incompatible type 'uint4' (vector of 4 'unsigned int' values)}}
+ int4 i = u; // expected-error{{initializing '__private int4' (vector of 4 'int' values) with an expression of incompatible type '__private uint4' (vector of 4 'unsigned int' values)}}
int4 e = (int4)u; // expected-error{{invalid conversion between ext-vector type 'int4' (vector of 4 'int' values) and 'uint4' (vector of 4 'unsigned int' values)}}
uint3 u4 = (uint3)u; // expected-error{{invalid conversion between ext-vector type 'uint3' (vector of 3 'unsigned int' values) and 'uint4' (vector of 4 'unsigned int' values)}}
Index: test/SemaOpenCL/vec_step.cl
===
--- test/SemaOpenCL/vec_step.cl
+++ test/SemaOpenCL/vec_step.cl
@@ -26,7 +26,7 @@
int res11[vec_step(int16) == 16 ? 1 : -1];
int res12[vec_step(void) == 1 ? 1 : -1];
- int res13 = vec_step(*incomplete1); // expected-error {{'vec_step' requires built-in scalar or vector type, 'struct S' invalid}}
- int res14 = vec_step(int16*); // expected-error {{'vec_step' requires built-in scalar or vector type, 'int16 *' invalid}}
+ int res13 = vec_step(*incomplete1); // expected-error {{'vec_step' requires built-in scalar or vector type, '__private struct S' invalid}}
+ int res14 = vec_step(int16 *); // expected-error {{'vec_step' requires built-in scalar or vector type, '__private int16 *' invalid}}
int res15 = vec_step(void(void)); // expected-error {{'vec_step' requires built-in scalar or vector type, 'void (void)' invalid}}
}
Index: test/SemaOpenCL/to_addr_builtin.cl
===
--- test/SemaOpenCL/to_addr_builtin.cl
+++ test/SemaOpenCL/to_addr_builtin.cl
@@ -11,45 +11,45 @@
glob = to_global(glob, loc);
#if __OPENCL_C_VERSION__ < CL_VERSION_2_0
// expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
- // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *' from 'int'}}
+ // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
#else
// expected-error@-5{{invalid number of arguments to function: 'to_global'}}
#endif
int x;
glob = to_global(x);
#if __OPENCL_C_VERSION__ < CL_VERSION_2_0
- // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__global int *' from 'int'}}
+ // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
#else
// expected-error@-4{{invalid argument x to function: 'to_global', expecting a generic pointer argument}}
#endif
glob = to_global(con);
#if __OPENCL_C_VERSION__ < CL_VERSION_2_0
- // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__global int *' from 'int'}}
+ // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
#else
// expected-error@-4{{invalid argument con to function: 'to_global', expecting a generic pointer argument}}
#endif
glob = to_global(con_typedef);
#if __OPENCL_C_VERSION__
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl marked 6 inline comments as done.
yaxunl added inline comments.
Comment at: lib/AST/Expr.cpp:3235
+// has non-default address space it is not treated as nullptr.
+bool PointeeHasDefaultAS = false;
+if (Ctx.getLangOpts().OpenCL)
Anastasia wrote:
> Would we still be accepting the following:
> global int * ptr = (global void*)0;
Yes. There is a test SemaOpenCL/null_literal.cl
Comment at: lib/Sema/SemaDecl.cpp:7964
+PointeeType.getAddressSpace() == LangAS::opencl_private ||
PointeeType.getAddressSpace() == 0)
return InvalidAddrSpacePtrKernelParam;
Anastasia wrote:
> Could we use `LangAS::Default` instead?
done
Comment at: lib/Sema/SemaDecl.cpp:11846
// an address space.
if (T.getAddressSpace() != 0) {
// OpenCL allows function arguments declared to be an array of a type
Anastasia wrote:
> Could we use `LangAS::Default` here too?
done
Comment at: lib/Sema/SemaDecl.cpp:11851
+ (T->isArrayType() ||
+ T.getAddressSpace() == LangAS::opencl_private))) {
Diag(NameLoc, diag::err_arg_with_address_space);
Anastasia wrote:
> Would it be better to lift this into if condition of line 11846?
will do.
Comment at: lib/Sema/SemaType.cpp:6969
+ if (state.getSema().getLangOpts().OpenCL &&
+ !hasOpenCLAddressSpace && type.getAddressSpace() == 0 &&
Anastasia wrote:
> Would it be nicer to not append any address space at all neither here nor
> down at the end of this function and keep it default instead until the
> Codegen? If it's doable I would very much prefer that. It seems like it would
> make the implementation potentially a bit cleaner to understand and easier to
> improve semantical analysis. See one example of improving original type
> printing in my comment to the test below.
>
> Also there are at least these 3 related bugs open currently:
> https://bugs.llvm.org//show_bug.cgi?id=33418
> https://bugs.llvm.org//show_bug.cgi?id=33419
> https://bugs.llvm.org//show_bug.cgi?id=33420
>
> Does your change address any of those?
On the contrary, I think using default address space for automatic variable and
function parameter will cause ambiguity and inconsistency in AST, making it
more difficult to understand and process, and making some bug (e.g.
https://bugs.llvm.org//show_bug.cgi?id=33419) unfixable. For example, `private
int f(void)` and `int f(void)` will be identical in AST, therefore we cannot
diagnose `private int f(void)`.
With current representation I am able to fix the 3 bugs. I will update the diff.
Comment at: test/SemaOpenCL/access-qualifier.cl:23
#else
-void myReadWrite(read_write image1d_t); // expected-error {{access qualifier
'read_write' can not be used for '__read_write image1d_t' prior to OpenCL
version 2.0}}
+void myReadWrite(read_write image1d_t); // expected-error {{access qualifier
'read_write' can not be used for '__private __read_write image1d_t' prior to
OpenCL version 2.0}}
#endif
Anastasia wrote:
> Ok, I think that here it would be less confusing not to add any address space
> since it's missing in the original source.
Although it looks strange at first sight, `__private __read_write image1d_t` is
the true type of the argument. The function argument is allocated from stack
and is an l-value. If we take its address, we get a private pointer.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
Anastasia added inline comments.
Comment at: lib/AST/Expr.cpp:3235
+// has non-default address space it is not treated as nullptr.
+bool PointeeHasDefaultAS = false;
+if (Ctx.getLangOpts().OpenCL)
Would we still be accepting the following:
global int * ptr = (global void*)0;
Comment at: lib/Sema/SemaDecl.cpp:7964
+PointeeType.getAddressSpace() == LangAS::opencl_private ||
PointeeType.getAddressSpace() == 0)
return InvalidAddrSpacePtrKernelParam;
Could we use `LangAS::Default` instead?
Comment at: lib/Sema/SemaDecl.cpp:11846
// an address space.
if (T.getAddressSpace() != 0) {
// OpenCL allows function arguments declared to be an array of a type
Could we use `LangAS::Default` here too?
Comment at: lib/Sema/SemaDecl.cpp:11851
+ (T->isArrayType() ||
+ T.getAddressSpace() == LangAS::opencl_private))) {
Diag(NameLoc, diag::err_arg_with_address_space);
Would it be better to lift this into if condition of line 11846?
Comment at: lib/Sema/SemaType.cpp:6969
+ if (state.getSema().getLangOpts().OpenCL &&
+ !hasOpenCLAddressSpace && type.getAddressSpace() == 0 &&
Would it be nicer to not append any address space at all neither here nor down
at the end of this function and keep it default instead until the Codegen? If
it's doable I would very much prefer that. It seems like it would make the
implementation potentially a bit cleaner to understand and easier to improve
semantical analysis. See one example of improving original type printing in my
comment to the test below.
Also there are at least these 3 related bugs open currently:
https://bugs.llvm.org//show_bug.cgi?id=33418
https://bugs.llvm.org//show_bug.cgi?id=33419
https://bugs.llvm.org//show_bug.cgi?id=33420
Does your change address any of those?
Comment at: test/SemaOpenCL/access-qualifier.cl:23
#else
-void myReadWrite(read_write image1d_t); // expected-error {{access qualifier
'read_write' can not be used for '__read_write image1d_t' prior to OpenCL
version 2.0}}
+void myReadWrite(read_write image1d_t); // expected-error {{access qualifier
'read_write' can not be used for '__private __read_write image1d_t' prior to
OpenCL version 2.0}}
#endif
Ok, I think that here it would be less confusing not to add any address space
since it's missing in the original source.
https://reviews.llvm.org/D35082
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
bader accepted this revision. bader added a comment. This revision is now accepted and ready to land. LGTM, thanks. https://reviews.llvm.org/D35082 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl updated this revision to Diff 105785.
yaxunl added a comment.
Treat (void*)0 as null pointer for OpenCL 1.2. Rebase to ToT.
https://reviews.llvm.org/D35082
Files:
include/clang/Basic/AddressSpaces.h
lib/AST/ASTContext.cpp
lib/AST/Expr.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/TypePrinter.cpp
lib/Basic/Targets.cpp
lib/CodeGen/CGDecl.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCL/address-spaces-mangling.cl
test/Index/opencl-types.cl
test/Parser/opencl-astype.cl
test/Parser/opencl-atomics-cl20.cl
test/SemaOpenCL/access-qualifier.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
test/SemaOpenCL/address-spaces.cl
test/SemaOpenCL/arithmetic-conversions.cl
test/SemaOpenCL/as_type.cl
test/SemaOpenCL/cl20-device-side-enqueue.cl
test/SemaOpenCL/event_t.cl
test/SemaOpenCL/extension-begin.cl
test/SemaOpenCL/half.cl
test/SemaOpenCL/images.cl
test/SemaOpenCL/invalid-block.cl
test/SemaOpenCL/invalid-image.cl
test/SemaOpenCL/invalid-kernel-parameters.cl
test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
test/SemaOpenCL/invalid-pipes-cl2.0.cl
test/SemaOpenCL/null_literal.cl
test/SemaOpenCL/null_queue.cl
test/SemaOpenCL/queue_t_overload.cl
test/SemaOpenCL/sampler_t.cl
test/SemaOpenCL/shifts.cl
test/SemaOpenCL/to_addr_builtin.cl
test/SemaOpenCL/vec_step.cl
test/SemaOpenCL/vector_conv_invalid.cl
Index: test/SemaOpenCL/vector_conv_invalid.cl
===
--- test/SemaOpenCL/vector_conv_invalid.cl
+++ test/SemaOpenCL/vector_conv_invalid.cl
@@ -7,7 +7,7 @@
void vector_conv_invalid() {
uint4 u = (uint4)(1);
- int4 i = u; // expected-error{{initializing 'int4' (vector of 4 'int' values) with an expression of incompatible type 'uint4' (vector of 4 'unsigned int' values)}}
+ int4 i = u; // expected-error{{initializing '__private int4' (vector of 4 'int' values) with an expression of incompatible type '__private uint4' (vector of 4 'unsigned int' values)}}
int4 e = (int4)u; // expected-error{{invalid conversion between ext-vector type 'int4' (vector of 4 'int' values) and 'uint4' (vector of 4 'unsigned int' values)}}
uint3 u4 = (uint3)u; // expected-error{{invalid conversion between ext-vector type 'uint3' (vector of 3 'unsigned int' values) and 'uint4' (vector of 4 'unsigned int' values)}}
Index: test/SemaOpenCL/vec_step.cl
===
--- test/SemaOpenCL/vec_step.cl
+++ test/SemaOpenCL/vec_step.cl
@@ -26,7 +26,7 @@
int res11[vec_step(int16) == 16 ? 1 : -1];
int res12[vec_step(void) == 1 ? 1 : -1];
- int res13 = vec_step(*incomplete1); // expected-error {{'vec_step' requires built-in scalar or vector type, 'struct S' invalid}}
- int res14 = vec_step(int16*); // expected-error {{'vec_step' requires built-in scalar or vector type, 'int16 *' invalid}}
+ int res13 = vec_step(*incomplete1); // expected-error {{'vec_step' requires built-in scalar or vector type, '__private struct S' invalid}}
+ int res14 = vec_step(int16 *); // expected-error {{'vec_step' requires built-in scalar or vector type, '__private int16 *' invalid}}
int res15 = vec_step(void(void)); // expected-error {{'vec_step' requires built-in scalar or vector type, 'void (void)' invalid}}
}
Index: test/SemaOpenCL/to_addr_builtin.cl
===
--- test/SemaOpenCL/to_addr_builtin.cl
+++ test/SemaOpenCL/to_addr_builtin.cl
@@ -11,45 +11,45 @@
glob = to_global(glob, loc);
#if __OPENCL_C_VERSION__ < CL_VERSION_2_0
// expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
- // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *' from 'int'}}
+ // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
#else
// expected-error@-5{{invalid number of arguments to function: 'to_global'}}
#endif
int x;
glob = to_global(x);
#if __OPENCL_C_VERSION__ < CL_VERSION_2_0
- // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__global int *' from 'int'}}
+ // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
#else
// expected-error@-4{{invalid argument x to function: 'to_global', expecting a generic pointer argument}}
#endif
glob = to_global(con);
#if __OPENCL_C_VERSION__ < CL_VERSION_2_0
- // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__global int *' from 'int'}}
+ // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
#else
// expected-error@-4{{invalid argument con to function: 'to_global', expecting a generic pointer argument}}
#endif
glob = to_global(con_typedef);
#if __OPENCL_C_VERSION__ <
[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST
yaxunl updated this revision to Diff 105774.
yaxunl retitled this revision from "[WIP][OpenCL] Add LangAS::opencl_private to
represent private address space in AST" to "[OpenCL] Add LangAS::opencl_private
to represent private address space in AST".
yaxunl edited the summary of this revision.
yaxunl added a comment.
Update lit tests.
It is essentially done. Clang already drops all qualifiers when converting
l-value to r-value.
The only missing part is to let Clang treat (void*)0 as a generic null pointer
for OpenCL 1.2 and below since now it is a pointer to private address space.
https://reviews.llvm.org/D35082
Files:
include/clang/Basic/AddressSpaces.h
lib/AST/ASTContext.cpp
lib/AST/ItaniumMangle.cpp
lib/AST/TypePrinter.cpp
lib/Basic/Targets.cpp
lib/CodeGen/CGDecl.cpp
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaType.cpp
test/CodeGenOpenCL/address-spaces-mangling.cl
test/Index/opencl-types.cl
test/Parser/opencl-astype.cl
test/Parser/opencl-atomics-cl20.cl
test/SemaOpenCL/access-qualifier.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
test/SemaOpenCL/address-spaces.cl
test/SemaOpenCL/arithmetic-conversions.cl
test/SemaOpenCL/as_type.cl
test/SemaOpenCL/cl20-device-side-enqueue.cl
test/SemaOpenCL/event_t.cl
test/SemaOpenCL/extension-begin.cl
test/SemaOpenCL/half.cl
test/SemaOpenCL/images.cl
test/SemaOpenCL/invalid-block.cl
test/SemaOpenCL/invalid-image.cl
test/SemaOpenCL/invalid-kernel-parameters.cl
test/SemaOpenCL/invalid-pipe-builtin-cl2.0.cl
test/SemaOpenCL/invalid-pipes-cl2.0.cl
test/SemaOpenCL/null_literal.cl
test/SemaOpenCL/null_queue.cl
test/SemaOpenCL/queue_t_overload.cl
test/SemaOpenCL/sampler_t.cl
test/SemaOpenCL/shifts.cl
test/SemaOpenCL/to_addr_builtin.cl
test/SemaOpenCL/vec_step.cl
test/SemaOpenCL/vector_conv_invalid.cl
Index: test/SemaOpenCL/vector_conv_invalid.cl
===
--- test/SemaOpenCL/vector_conv_invalid.cl
+++ test/SemaOpenCL/vector_conv_invalid.cl
@@ -7,7 +7,7 @@
void vector_conv_invalid() {
uint4 u = (uint4)(1);
- int4 i = u; // expected-error{{initializing 'int4' (vector of 4 'int' values) with an expression of incompatible type 'uint4' (vector of 4 'unsigned int' values)}}
+ int4 i = u; // expected-error{{initializing '__private int4' (vector of 4 'int' values) with an expression of incompatible type '__private uint4' (vector of 4 'unsigned int' values)}}
int4 e = (int4)u; // expected-error{{invalid conversion between ext-vector type 'int4' (vector of 4 'int' values) and 'uint4' (vector of 4 'unsigned int' values)}}
uint3 u4 = (uint3)u; // expected-error{{invalid conversion between ext-vector type 'uint3' (vector of 3 'unsigned int' values) and 'uint4' (vector of 4 'unsigned int' values)}}
Index: test/SemaOpenCL/vec_step.cl
===
--- test/SemaOpenCL/vec_step.cl
+++ test/SemaOpenCL/vec_step.cl
@@ -26,7 +26,7 @@
int res11[vec_step(int16) == 16 ? 1 : -1];
int res12[vec_step(void) == 1 ? 1 : -1];
- int res13 = vec_step(*incomplete1); // expected-error {{'vec_step' requires built-in scalar or vector type, 'struct S' invalid}}
- int res14 = vec_step(int16*); // expected-error {{'vec_step' requires built-in scalar or vector type, 'int16 *' invalid}}
+ int res13 = vec_step(*incomplete1); // expected-error {{'vec_step' requires built-in scalar or vector type, '__private struct S' invalid}}
+ int res14 = vec_step(int16 *); // expected-error {{'vec_step' requires built-in scalar or vector type, '__private int16 *' invalid}}
int res15 = vec_step(void(void)); // expected-error {{'vec_step' requires built-in scalar or vector type, 'void (void)' invalid}}
}
Index: test/SemaOpenCL/to_addr_builtin.cl
===
--- test/SemaOpenCL/to_addr_builtin.cl
+++ test/SemaOpenCL/to_addr_builtin.cl
@@ -11,45 +11,45 @@
glob = to_global(glob, loc);
#if __OPENCL_C_VERSION__ < CL_VERSION_2_0
// expected-error@-2{{implicit declaration of function 'to_global' is invalid in OpenCL}}
- // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *' from 'int'}}
+ // expected-warning@-3{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
#else
// expected-error@-5{{invalid number of arguments to function: 'to_global'}}
#endif
int x;
glob = to_global(x);
#if __OPENCL_C_VERSION__ < CL_VERSION_2_0
- // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__global int *' from 'int'}}
+ // expected-warning@-2{{incompatible integer to pointer conversion assigning to '__global int *__private' from 'int'}}
#else
// expected-error@-4{{invalid argument x to function: 'to_global', expecting a generic pointer argument}}
#endif
glob = to_global(con);
#if __OPENCL_C_VERSION__ <
