Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Closed by commit rL266111: PR19957: [OpenCL] Incorrectly accepts implicit
address space conversion with… (authored by yaxunl).
Changed prior to commit:
http://reviews.llvm.org/D17412?vs=52080=53445#toc
Repository:
rL LLVM
http://reviews.llvm.org/D17412
Files:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/AST/ASTContext.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/test/CodeGenOpenCL/address-spaces-conversions.cl
cfe/trunk/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
Index: cfe/trunk/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
===
--- cfe/trunk/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
+++ cfe/trunk/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
@@ -225,3 +225,69 @@
// expected-error@-2{{passing '__constant int *' to parameter of type '__generic int *' changes address space of pointer}}
#endif
}
+
+void test_ternary() {
+ AS int *var_cond;
+ generic int *var_gen;
+ global int *var_glob;
+ var_gen = 0 ? var_cond : var_glob;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ local int *var_loc;
+ var_gen = 0 ? var_cond : var_loc;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and '__local int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ constant int *var_const;
+ var_cond = 0 ? var_cond : var_const;
+#ifndef CONSTANT
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|generic}} int *' and '__constant int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ private int *var_priv;
+ var_gen = 0 ? var_cond : var_priv;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and 'int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ var_gen = 0 ? var_cond : var_gen;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__generic int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ void *var_void_gen;
+ global char *var_glob_ch;
+ var_void_gen = 0 ? var_cond : var_glob_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ local char *var_loc_ch;
+ var_void_gen = 0 ? var_cond : var_loc_ch;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and '__local char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ constant void *var_void_const;
+ constant char *var_const_ch;
+ var_void_const = 0 ? var_cond : var_const_ch;
+#ifndef CONSTANT
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|generic}} int *' and '__constant char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ private char *var_priv_ch;
+ var_void_gen = 0 ? var_cond : var_priv_ch;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and 'char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ generic char *var_gen_ch;
+ var_void_gen = 0 ? var_cond : var_gen_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__generic char *') which are pointers to non-overlapping address spaces}}
+#endif
+}
+
Index: cfe/trunk/test/CodeGenOpenCL/address-spaces-conversions.cl
===
--- cfe/trunk/test/CodeGenOpenCL/address-spaces-conversions.cl
+++ cfe/trunk/test/CodeGenOpenCL/address-spaces-conversions.cl
@@ -5,6 +5,7 @@
// test that we generate address space casts everywhere we need conversions of
// pointers to different address spaces
+// CHECK: define void @test
void test(global int *arg_glob, generic int *arg_gen) {
int var_priv;
arg_gen = arg_glob; // implicit cast global -> generic
@@ -39,3 +40,41 @@
// CHECK-NOFAKE: bitcast
// CHECK-NOFAKE-NOT: addrspacecast
}
+
+// Test ternary operator.
+// CHECK: define void @test_ternary
+void test_ternary(void) {
+ global int *var_glob;
+ generic int *var_gen;
+ generic int *var_gen2;
+ generic float *var_gen_f;
+ generic void *var_gen_v;
+
+ var_gen = var_gen ? var_gen : var_gen2; // operands of the same
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
Anastasia added a comment. In http://reviews.llvm.org/D17412#391351, @Matt wrote: > In http://reviews.llvm.org/D17412#391322, @Anastasia wrote: > > > Cool, thanks! I think we should commit this ASAP. > > > > @Xiuli/@Matt, do you have any more comments here? > > > Hi! I think that "Matt" for this one would be @arsenm :-) Oh, sure. Thanks for pointing this out!!! @arsenm, if you don't have any objections, could we commit this? http://reviews.llvm.org/D17412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
pxli168 accepted this revision. pxli168 added a comment. LGTM! Thanks! http://reviews.llvm.org/D17412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
Matt added a comment. In http://reviews.llvm.org/D17412#391322, @Anastasia wrote: > Cool, thanks! I think we should commit this ASAP. > > @Xiuli/@Matt, do you have any more comments here? Hi! I think that "Matt" for this one would be @arsenm :-) http://reviews.llvm.org/D17412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
Anastasia added a subscriber: Matt. Anastasia added a comment. Cool, thanks! I think we should commit this ASAP. @Xiuli/@Matt, do you have any more comments here? http://reviews.llvm.org/D17412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
yaxunl marked 2 inline comments as done. Comment at: test/CodeGenOpenCL/address-spaces-conversions.cl:1 @@ -1,2 +1,2 @@ // RUN: %clang_cc1 %s -triple x86_64-unknown-linux-gnu -O0 -ffake-address-space-map -cl-std=CL2.0 -emit-llvm -o - | FileCheck %s Anastasia wrote: > Cool, thanks! Could you insert a link to the new review here if possible. I looked into the ICE when -ffake-address-space-map is not set. when -ffake-address-space-map is set, address space 0-4 mapped to 0-4. When it is not set, it maps to values defined by target. x86 uses DefaultAddrSpaceMap which maps 0-4 all to 0. This causes assertion for testcase like global int *a; generic void *b = a; A review is opened: http://reviews.llvm.org/D18713 http://reviews.llvm.org/D17412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
yaxunl updated this revision to Diff 52080.
yaxunl marked 3 inline comments as done.
yaxunl added a comment.
Add back a blank line deleted by accident.
Add OpenCL to a comment.
http://reviews.llvm.org/D17412
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/ASTContext.cpp
lib/Sema/SemaExpr.cpp
test/CodeGenOpenCL/address-spaces-conversions.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
Index: test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
===
--- test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
+++ test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
@@ -225,3 +225,69 @@
// expected-error@-2{{passing '__constant int *' to parameter of type '__generic int *' changes address space of pointer}}
#endif
}
+
+void test_ternary() {
+ AS int *var_cond;
+ generic int *var_gen;
+ global int *var_glob;
+ var_gen = 0 ? var_cond : var_glob;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ local int *var_loc;
+ var_gen = 0 ? var_cond : var_loc;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and '__local int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ constant int *var_const;
+ var_cond = 0 ? var_cond : var_const;
+#ifndef CONSTANT
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|generic}} int *' and '__constant int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ private int *var_priv;
+ var_gen = 0 ? var_cond : var_priv;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and 'int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ var_gen = 0 ? var_cond : var_gen;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__generic int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ void *var_void_gen;
+ global char *var_glob_ch;
+ var_void_gen = 0 ? var_cond : var_glob_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ local char *var_loc_ch;
+ var_void_gen = 0 ? var_cond : var_loc_ch;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and '__local char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ constant void *var_void_const;
+ constant char *var_const_ch;
+ var_void_const = 0 ? var_cond : var_const_ch;
+#ifndef CONSTANT
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|generic}} int *' and '__constant char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ private char *var_priv_ch;
+ var_void_gen = 0 ? var_cond : var_priv_ch;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and 'char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ generic char *var_gen_ch;
+ var_void_gen = 0 ? var_cond : var_gen_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__generic char *') which are pointers to non-overlapping address spaces}}
+#endif
+}
+
Index: test/CodeGenOpenCL/address-spaces-conversions.cl
===
--- test/CodeGenOpenCL/address-spaces-conversions.cl
+++ test/CodeGenOpenCL/address-spaces-conversions.cl
@@ -3,20 +3,65 @@
// test that we generate address space casts everywhere we need conversions of
// pointers to different address spaces
+// CHECK: define void @test
void test(global int *arg_glob, generic int *arg_gen) {
int var_priv;
+
arg_gen = arg_glob; // implicit cast global -> generic
// CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(1)* %{{[0-9]+}} to i32 addrspace(4)*
+
arg_gen = _priv; // implicit cast with obtaining adr, private -> generic
// CHECK: %{{[0-9]+}} = addrspacecast i32* %var_priv to i32 addrspace(4)*
+
arg_glob = (global int *)arg_gen; // explicit cast
// CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(4)* %{{[0-9]+}} to i32 addrspace(1)*
+
global int *var_glob =
(global int *)arg_glob; // explicit cast in the same address space
// CHECK-NOT: %{{[0-9]+}} = addrspacecast i32 addrspace(1)* %{{[0-9]+}} to i32 addrspace(1)*
+
var_priv = arg_gen - arg_glob; // arithmetic
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.
LGTM, please correct two small issues commented here!
Comment at: lib/Sema/SemaExpr.cpp:169
@@ -168,4 +168,3 @@
break;
-
bool Warn = !D->getAttr()->isInherited();
// Objective-C method declarations in categories are not modelled as
Why removing line here?
Comment at: lib/Sema/SemaExpr.cpp:6192
@@ -6181,2 +6191,3 @@
+ // Cases 1c, 2a, 2b, and 2c.
if (CompositeTy.isNull()) {
add OpenCL
http://reviews.llvm.org/D17412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
yaxunl updated this revision to Diff 52073.
yaxunl marked 6 inline comments as done.
yaxunl added a comment.
Revised by Anastasia's comments.
Added comments to code for cases 1a-c and 2a-c.
Added codegen test for missing cases of 1a-b and 2a-b.
Rename variables in sema test.
http://reviews.llvm.org/D17412
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/ASTContext.cpp
lib/Sema/SemaExpr.cpp
test/CodeGenOpenCL/address-spaces-conversions.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
Index: test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
===
--- test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
+++ test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
@@ -225,3 +225,69 @@
// expected-error@-2{{passing '__constant int *' to parameter of type '__generic int *' changes address space of pointer}}
#endif
}
+
+void test_ternary() {
+ AS int *var_cond;
+ generic int *var_gen;
+ global int *var_glob;
+ var_gen = 0 ? var_cond : var_glob;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ local int *var_loc;
+ var_gen = 0 ? var_cond : var_loc;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and '__local int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ constant int *var_const;
+ var_cond = 0 ? var_cond : var_const;
+#ifndef CONSTANT
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|generic}} int *' and '__constant int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ private int *var_priv;
+ var_gen = 0 ? var_cond : var_priv;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and 'int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ var_gen = 0 ? var_cond : var_gen;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__generic int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ void *var_void_gen;
+ global char *var_glob_ch;
+ var_void_gen = 0 ? var_cond : var_glob_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ local char *var_loc_ch;
+ var_void_gen = 0 ? var_cond : var_loc_ch;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and '__local char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ constant void *var_void_const;
+ constant char *var_const_ch;
+ var_void_const = 0 ? var_cond : var_const_ch;
+#ifndef CONSTANT
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|generic}} int *' and '__constant char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ private char *var_priv_ch;
+ var_void_gen = 0 ? var_cond : var_priv_ch;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and 'char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ generic char *var_gen_ch;
+ var_void_gen = 0 ? var_cond : var_gen_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__generic char *') which are pointers to non-overlapping address spaces}}
+#endif
+}
+
Index: test/CodeGenOpenCL/address-spaces-conversions.cl
===
--- test/CodeGenOpenCL/address-spaces-conversions.cl
+++ test/CodeGenOpenCL/address-spaces-conversions.cl
@@ -3,20 +3,65 @@
// test that we generate address space casts everywhere we need conversions of
// pointers to different address spaces
+// CHECK: define void @test
void test(global int *arg_glob, generic int *arg_gen) {
int var_priv;
+
arg_gen = arg_glob; // implicit cast global -> generic
// CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(1)* %{{[0-9]+}} to i32 addrspace(4)*
+
arg_gen = _priv; // implicit cast with obtaining adr, private -> generic
// CHECK: %{{[0-9]+}} = addrspacecast i32* %var_priv to i32 addrspace(4)*
+
arg_glob = (global int *)arg_gen; // explicit cast
// CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(4)* %{{[0-9]+}} to i32 addrspace(1)*
+
global int *var_glob =
(global int *)arg_glob; // explicit cast in the same address space
// CHECK-NOT: %{{[0-9]+}} = addrspacecast i32
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
yaxunl added inline comments. Comment at: test/SemaOpenCL/address-spaces-conversions-cl2.0.cl:276 @@ +275,3 @@ + constant char *arg_const_ch; + var_void_const = 0 ? var_cond : arg_const_ch; +#ifndef CONSTANT Anastasia wrote: > btw, what happens if we assign into non void* var? Do we get another error? No error, since void pointer can be casted to non-void pointer implicitly in C. http://reviews.llvm.org/D17412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
yaxunl marked 6 inline comments as done.
Comment at: lib/AST/ASTContext.cpp:7613
@@ +7612,3 @@
+if (getLangOpts().OpenCL) {
+ if (LHS.getUnqualifiedType() != RHS.getUnqualifiedType() ||
+ LQuals.getCVRQualifiers() != RQuals.getCVRQualifiers())
Anastasia wrote:
> > Here if unqualified types are different
>
> I think this check is redundant considering that we make check of canonical
> types equivalence in line 7605. Also it doesn't really have anything to do
> with any OpenCL specific rule. Therefore I would remove this check and just
> merge with lines 7623 - 7624 as much as possible.
>
> > or CVS qualifiers are different, the two types cannot be merged
>
> The same is already being checked in line 7623. Could we merge with that code?
>
>
The check for unqualified type is not redundant.
Let's say global int and generic float gets here. If we don't check unqualified
type, we will get a non-null merged type, which is not correct.
It seems to be cleaner to keep the OpenCL logic separate from line 7623.
http://reviews.llvm.org/D17412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
Anastasia added a comment.
LG, apart from small comments mentioned here.
Comment at: lib/AST/ASTContext.cpp:7613
@@ +7612,3 @@
+if (getLangOpts().OpenCL) {
+ if (LHS.getUnqualifiedType() != RHS.getUnqualifiedType() ||
+ LQuals.getCVRQualifiers() != RQuals.getCVRQualifiers())
> Here if unqualified types are different
I think this check is redundant considering that we make check of canonical
types equivalence in line 7605. Also it doesn't really have anything to do with
any OpenCL specific rule. Therefore I would remove this check and just merge
with lines 7623 - 7624 as much as possible.
> or CVS qualifiers are different, the two types cannot be merged
The same is already being checked in line 7623. Could we merge with that code?
Comment at: lib/Sema/SemaExpr.cpp:6171
@@ +6170,3 @@
+ // (b) AS overlap => generate addrspacecast
+ // (c) As don't overlap => give an error
+ // 2. if LHS and RHS types don't match:
As -> AS
Comment at: lib/Sema/SemaExpr.cpp:6186
@@ +6185,3 @@
+ // spaces is disallowed.
+ // OpenCL v2.0 s6.5.6 - Clause 6.5.15 Conditional operator, add another
+ // constraint paragraph: If the second and third operands are pointers
Could you remove "Conditional operator, add another constraint paragraph: "
Comment at: lib/Sema/SemaExpr.cpp:6190
@@ +6189,3 @@
+ unsigned ResultAddrSpace;
+ if (lhQual.isAddressSpaceSupersetOf(rhQual)) {
+ResultAddrSpace = lhQual.getAddressSpace();
Could you add a comment referring to the case from 1(a-c)&2(a-c) you are
handling here!
Comment at: lib/Sema/SemaExpr.cpp:6229
@@ +6228,3 @@
+ else {
+auto ResultAddrSpace = ResultTy.getQualifiers().getAddressSpace();
+LHSCastKind = lhQual.getAddressSpace() == ResultAddrSpace
The same here - could you add a comment explaining the case from 1(a-c)&2(a-c)
being handled!
Comment at: test/CodeGenOpenCL/address-spaces-conversions.cl:1
@@ -1,2 +1,2 @@
// RUN: %clang_cc1 %s -triple x86_64-unknown-linux-gnu -O0
-ffake-address-space-map -cl-std=CL2.0 -emit-llvm -o - | FileCheck %s
Cool, thanks! Could you insert a link to the new review here if possible.
Comment at: test/CodeGenOpenCL/address-spaces-conversions.cl:37
@@ -22,1 +36,2 @@
+ // CHECK: %{{[0-9]+}} = addrspacecast float addrspace(1)* %{{[0-9]+}} to i8
addrspace(4)*
}
Could we also add case 1a and 2a to test that we don't affect standard cases.
Comment at: test/SemaOpenCL/address-spaces-conversions-cl2.0.cl:231
@@ +230,3 @@
+ AS int *var_cond;
+ generic int *arg_gen;
+ global int *arg_glob;
arg_gen -> var_gen
arg_glob -> var_glob
Comment at: test/SemaOpenCL/address-spaces-conversions-cl2.0.cl:262
@@ +261,3 @@
+ void *var_void_gen;
+ global char *arg_glob_ch;
+ var_void_gen = 0 ? var_cond : arg_glob_ch;
I think you should change all arg to var in the name as it mean argument, but
it's just a variable now.
Comment at: test/SemaOpenCL/address-spaces-conversions-cl2.0.cl:276
@@ +275,3 @@
+ constant char *arg_const_ch;
+ var_void_const = 0 ? var_cond : arg_const_ch;
+#ifndef CONSTANT
btw, what happens if we assign into non void* var? Do we get another error?
http://reviews.llvm.org/D17412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
yaxunl removed rL LLVM as the repository for this revision.
yaxunl updated this revision to Diff 51460.
yaxunl marked 13 inline comments as done.
yaxunl added a comment.
Added comments. Revised test.
http://reviews.llvm.org/D17412
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/ASTContext.cpp
lib/Sema/SemaExpr.cpp
test/CodeGenOpenCL/address-spaces-conversions.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
Index: test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
===
--- test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
+++ test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
@@ -225,3 +225,69 @@
// expected-error@-2{{passing '__constant int *' to parameter of type '__generic int *' changes address space of pointer}}
#endif
}
+
+void test_ternary() {
+ AS int *var_cond;
+ generic int *arg_gen;
+ global int *arg_glob;
+ arg_gen = 0 ? var_cond : arg_glob;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ local int *arg_loc;
+ arg_gen = 0 ? var_cond : arg_loc;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and '__local int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ constant int *arg_const;
+ var_cond = 0 ? var_cond : arg_const;
+#ifndef CONSTANT
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|generic}} int *' and '__constant int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ private int *arg_priv;
+ arg_gen = 0 ? var_cond : arg_priv;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and 'int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ arg_gen = 0 ? var_cond : arg_gen;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__generic int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ void *var_void_gen;
+ global char *arg_glob_ch;
+ var_void_gen = 0 ? var_cond : arg_glob_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ local char *arg_loc_ch;
+ var_void_gen = 0 ? var_cond : arg_loc_ch;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and '__local char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ constant void *var_void_const;
+ constant char *arg_const_ch;
+ var_void_const = 0 ? var_cond : arg_const_ch;
+#ifndef CONSTANT
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|generic}} int *' and '__constant char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ private char *arg_priv_ch;
+ var_void_gen = 0 ? var_cond : arg_priv_ch;
+#ifndef GENERIC
+// expected-error-re@-2{{conditional operator with the second and third operands of type ('__{{global|constant}} int *' and 'char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ generic char *arg_gen_ch;
+ var_void_gen = 0 ? var_cond : arg_gen_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__generic char *') which are pointers to non-overlapping address spaces}}
+#endif
+}
+
Index: test/CodeGenOpenCL/address-spaces-conversions.cl
===
--- test/CodeGenOpenCL/address-spaces-conversions.cl
+++ test/CodeGenOpenCL/address-spaces-conversions.cl
@@ -3,20 +3,35 @@
// test that we generate address space casts everywhere we need conversions of
// pointers to different address spaces
-void test(global int *arg_glob, generic int *arg_gen) {
+void test(global int *arg_glob, generic int *arg_gen, global float *arg_glob_f,
+ generic void *arg_gen_v) {
int var_priv;
+
arg_gen = arg_glob; // implicit cast global -> generic
// CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(1)* %{{[0-9]+}} to i32 addrspace(4)*
+
arg_gen = _priv; // implicit cast with obtaining adr, private -> generic
// CHECK: %{{[0-9]+}} = addrspacecast i32* %var_priv to i32 addrspace(4)*
+
arg_glob = (global int *)arg_gen; // explicit cast
// CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(4)* %{{[0-9]+}} to i32 addrspace(1)*
+
global int *var_glob =
(global int *)arg_glob; // explicit cast in the same address space
// CHECK-NOT: %{{[0-9]+}} =
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
pxli168 added inline comments.
Comment at: lib/AST/ASTContext.cpp:7613
@@ +7612,3 @@
+if (getLangOpts().OpenCL) {
+ if (LHS.getUnqualifiedType() != RHS.getUnqualifiedType() ||
+ LQuals.getCVRQualifiers() != RQuals.getCVRQualifiers())
Anastasia wrote:
> Do you think we should check the types here? I was thinking we should do the
> check exactly as below apart from AS specific part.
I think the mergeType function it very complex, too. It seems to check type can
be merged recursively later.
Comment at: lib/Sema/SemaExpr.cpp:6168
@@ -6168,3 +6167,3 @@
QualType CompositeTy = S.Context.mergeTypes(lhptee, rhptee);
if (CompositeTy.isNull()) {
Anastasia wrote:
> Could we instead add a comment explaining different cases with AS we can have
> here i.e. 1(a-c)&2(a-c)!
>
> And may be we could refer to each case by adding comments in code below.
Good idea.
Comment at: lib/Sema/SemaExpr.cpp:6222-6227
@@ -6188,1 +6221,8 @@
+auto ResultAddrSpace = ResultTy.getQualifiers().getAddressSpace();
+LHSCastKind = lhQual.getAddressSpace() == ResultAddrSpace
+ ? CK_BitCast
+ : CK_AddressSpaceConversion;
+RHSCastKind = rhQual.getAddressSpace() == ResultAddrSpace
+ ? CK_BitCast
+ : CK_AddressSpaceConversion;
ResultTy = S.Context.getPointerType(ResultTy);
Anastasia wrote:
> pxli168 wrote:
> > What will mergetypes return?
> > It seems the LHS and RHS are compatibel here, and may be they did not need
> > bitcast?
> I think we always need a cast here, because types are not exactly the same at
> this point!
I tried to figure out what mergetypes will return and find it seems to have
logic far more complex than this.
Repository:
rL LLVM
http://reviews.llvm.org/D17412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
pxli168 added a comment. The logic is still to complex and I hope it can be optimized. Comment at: lib/Sema/SemaExpr.cpp:6222-6227 @@ -6188,1 +6221,8 @@ +auto ResultAddrSpace = ResultTy.getQualifiers().getAddressSpace(); +LHSCastKind = lhQual.getAddressSpace() == ResultAddrSpace + ? CK_BitCast + : CK_AddressSpaceConversion; +RHSCastKind = rhQual.getAddressSpace() == ResultAddrSpace + ? CK_BitCast + : CK_AddressSpaceConversion; ResultTy = S.Context.getPointerType(ResultTy); What will mergetypes return? It seems the LHS and RHS are compatibel here, and may be they did not need bitcast? Repository: rL LLVM http://reviews.llvm.org/D17412 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
yaxunl added inline comments.
Comment at: lib/AST/ASTContext.cpp:7605
@@ -7604,3 +7604,3 @@
// If two types are identical, they are compatible.
if (LHSCan == RHSCan)
return LHS;
Anastasia wrote:
> I feel like the AS check should be lifted here instead, because here we check
> unqualified types and then return LHS type. In OpenCL we have to return
> either LHS or RHS type just like you do below if AS overlap.
Here is still checking qualified type since 'Unqualified' is false. So this
condition is for the case when the two types are the same.
Comment at: lib/AST/ASTContext.cpp:7624
@@ -7614,3 +7623,3 @@
if (LQuals.getCVRQualifiers() != RQuals.getCVRQualifiers() ||
LQuals.getAddressSpace() != RQuals.getAddressSpace() ||
LQuals.getObjCLifetime() != RQuals.getObjCLifetime())
Anastasia wrote:
> We should add !OpenCL here. Because for OpenCL this check is wrong to do.
>
> But I am not sure whether we need to add an OpenCL check here though, as we
> don't seem to return any type but void for OpenCL after this statement
> anyways.
>
> However, we might return 'generic void*' if AS overlap, instead of 'private
> void*' as we do now. Would this make more sense?
>
>
I think I need to handle all OpenCL cases above.
1. if types match, return LHS
2. if types differ, but addr spaces overlap and cvs qual match, return type
with bigger addr space
3. otherwise return empty type
then we don't need check OpenCL here.
Comment at: lib/Sema/SemaExpr.cpp:6182
@@ +6181,3 @@
+ unsigned ResultAddrSpace;
+ if (lhQual.isAddressSpaceSupersetOf(rhQual)) {
+ResultAddrSpace = lhQual.getAddressSpace();
Anastasia wrote:
> if we return generic pointer type in mergeTypes, we won't need
> ResultAddrSpace as it will be returned in CompisiteTy.
This part handles the case when mergeTypes() returns an empty type. Since the
returned type is empty, we still need to find the result addr space.
This happens if the addr spaces are not overlapping or the unqualified pointee
types are different.
For non-OpenCL case, Clang will insert implicit cast to void* for the two
operands.
For OpenCL, we check the addr spaces. If they overlap, that means the
unqualified pointee types are different, e.g.
global int* a;
generic char *b;
0?a:b;
in this case, to mimic the original Clang behavior, we insert casts so that we
get 0?(generic void*)a:(generic void*)b.
Comment at: lib/Sema/SemaExpr.cpp:6194-6203
@@ +6193,12 @@
+
+ incompatTy = S.Context.getPointerType(
+ S.Context.getAddrSpaceQualType(S.Context.VoidTy, ResultAddrSpace));
+ LHS = S.ImpCastExprToType(LHS.get(), incompatTy,
+(lhQual.getAddressSpace() != ResultAddrSpace)
+? CK_AddressSpaceConversion
+: CK_BitCast);
+ RHS = S.ImpCastExprToType(RHS.get(), incompatTy,
+(rhQual.getAddressSpace() != ResultAddrSpace)
+? CK_AddressSpaceConversion
+: CK_BitCast);
+} else {
Anastasia wrote:
> yaxunl wrote:
> > pxli168 wrote:
> > > I am quite confused by these codes. It seems in some situations you need
> > > both BitCast and AddressSpaceConversion.
> > > It seems the logic here is too complex. Maybe you can try to simplify it
> > >
> > if the addr space is different, CK_AddressSpaceConversion is used, which
> > corresponds to addrspacecast in LLVM (it is OK if pointee base types are
> > different here, addrspacecast covers that, no need for an extra bitcast).
> >
> > I've tried to simplify the logic. Any suggestion how to further simplify
> > the logic?
> >
> Yes, it's a bit complicated here because we are trying to extend C rules.
>
> In general we might have the following situations:
>
> 1. If LHS and RHS types match exactly and:
> (a) AS match => use standard C rules, no bitcast or addrspacecast
> (b) AS overlap => generate addrspacecast
> (c) As don't overlap => give an error
> 2. if LHS and RHS types don't match:
> (a) AS match => use standard C rules, generate bitcast
> (b) AS overlap => generate addrspacecast instead of bitcast
> (c) AS don't overlap => give an error
>
> I think however we are missing testing all of the cases at the moment. Could
> you please add more tests!
I think we are missing tests for 2a 2b 2c. I will add them.
Repository:
rL LLVM
http://reviews.llvm.org/D17412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
yaxunl updated this revision to Diff 49232.
yaxunl marked 7 inline comments as done.
yaxunl added a comment.
Revised as Anastasis suggested.
Modified mergeTypes() for un-handled case.
Separate sema tests for condition operator to a new file.
Repository:
rL LLVM
http://reviews.llvm.org/D17412
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/ASTContext.cpp
lib/Sema/SemaExpr.cpp
test/CodeGenOpenCL/address-spaces-conversions.cl
test/SemaOpenCL/condition-operator-cl2.0.cl
Index: test/SemaOpenCL/condition-operator-cl2.0.cl
===
--- /dev/null
+++ test/SemaOpenCL/condition-operator-cl2.0.cl
@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 %s -ffake-address-space-map -verify -pedantic -fsyntax-only -DCONSTANT -cl-std=CL2.0
+// RUN: %clang_cc1 %s -ffake-address-space-map -verify -pedantic -fsyntax-only -DGLOBAL -cl-std=CL2.0
+// RUN: %clang_cc1 %s -ffake-address-space-map -verify -pedantic -fsyntax-only -DGENERIC -cl-std=CL2.0
+
+// Testing conditional operator with second and third operands of pointer types.
+
+#ifdef GENERIC
+#define AS generic
+#endif
+
+#ifdef GLOBAL
+#define AS global
+#endif
+
+#ifdef CONSTANT
+#define AS constant
+#endif
+
+void test_conversion(global int *arg_glob, local int *arg_loc,
+ constant int *arg_const, private int *arg_priv,
+ generic int *arg_gen, global char *arg_glob_ch,
+ local char *arg_loc_ch, constant char *arg_const_ch,
+ private char *arg_priv_ch, generic char *arg_gen_ch) {
+
+ AS int *var_cond;
+ arg_gen = 0 ? var_cond : arg_glob;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ arg_gen = 0 ? var_cond : arg_loc;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__local int *') which are pointers to non-overlapping address spaces}}
+#elif defined(GLOBAL)
+// expected-error@-4{{conditional operator with the second and third operands of type ('__global int *' and '__local int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ var_cond = 0 ? var_cond : arg_const;
+#ifdef GENERIC
+// expected-error@-2{{conditional operator with the second and third operands of type ('__generic int *' and '__constant int *') which are pointers to non-overlapping address spaces}}
+#elif defined(GLOBAL)
+// expected-error@-4{{conditional operator with the second and third operands of type ('__global int *' and '__constant int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ arg_gen = 0 ? var_cond : arg_priv;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and 'int *') which are pointers to non-overlapping address spaces}}
+#elif defined(GLOBAL)
+// expected-error@-4{{conditional operator with the second and third operands of type ('__global int *' and 'int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ arg_gen = 0 ? var_cond : arg_gen;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__generic int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ void *var_void_gen;
+ constant void*var_void_const;
+ var_void_gen = 0 ? var_cond : arg_glob_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ var_void_gen = 0 ? var_cond : arg_loc_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__local char *') which are pointers to non-overlapping address spaces}}
+#elif defined(GLOBAL)
+// expected-error@-4{{conditional operator with the second and third operands of type ('__global int *' and '__local char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ var_void_const = 0 ? var_cond : arg_const_ch;
+#ifdef GENERIC
+// expected-error@-2{{conditional operator with the second and third operands of type ('__generic int *' and '__constant char *') which are pointers to non-overlapping address spaces}}
+#elif defined(GLOBAL)
+// expected-error@-4{{conditional operator with the second and third operands of type ('__global int *' and '__constant char *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ var_void_gen = 0 ? var_cond : arg_priv_ch;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and 'char *') which are pointers to non-overlapping address spaces}}
+#elif defined(GLOBAL)
+//
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
Anastasia added a comment.
I think we are not covering all the possible cases with tests now! May be we
could also create a separate cl file since it becomes quite large.
Comment at: lib/AST/ASTContext.cpp:7605
@@ -7604,3 +7604,3 @@
// If two types are identical, they are compatible.
if (LHSCan == RHSCan)
return LHS;
I feel like the AS check should be lifted here instead, because here we check
unqualified types and then return LHS type. In OpenCL we have to return either
LHS or RHS type just like you do below if AS overlap.
Comment at: lib/AST/ASTContext.cpp:7616
@@ +7615,3 @@
+ if (LQuals.isAddressSpaceSupersetOf(RQuals))
+return LHS;
+ if (RQuals.isAddressSpaceSupersetOf(LQuals))
I think this should be rather done above (see comment to line 7605) w/o CVR
Quals check.
Comment at: lib/AST/ASTContext.cpp:7624
@@ -7614,3 +7623,3 @@
if (LQuals.getCVRQualifiers() != RQuals.getCVRQualifiers() ||
LQuals.getAddressSpace() != RQuals.getAddressSpace() ||
LQuals.getObjCLifetime() != RQuals.getObjCLifetime())
We should add !OpenCL here. Because for OpenCL this check is wrong to do.
But I am not sure whether we need to add an OpenCL check here though, as we
don't seem to return any type but void for OpenCL after this statement anyways.
However, we might return 'generic void*' if AS overlap, instead of 'private
void*' as we do now. Would this make more sense?
Comment at: lib/Sema/SemaExpr.cpp:6182
@@ +6181,3 @@
+ unsigned ResultAddrSpace;
+ if (lhQual.isAddressSpaceSupersetOf(rhQual)) {
+ResultAddrSpace = lhQual.getAddressSpace();
if we return generic pointer type in mergeTypes, we won't need ResultAddrSpace
as it will be returned in CompisiteTy.
Comment at: lib/Sema/SemaExpr.cpp:6194-6203
@@ +6193,12 @@
+
+ incompatTy = S.Context.getPointerType(
+ S.Context.getAddrSpaceQualType(S.Context.VoidTy, ResultAddrSpace));
+ LHS = S.ImpCastExprToType(LHS.get(), incompatTy,
+(lhQual.getAddressSpace() != ResultAddrSpace)
+? CK_AddressSpaceConversion
+: CK_BitCast);
+ RHS = S.ImpCastExprToType(RHS.get(), incompatTy,
+(rhQual.getAddressSpace() != ResultAddrSpace)
+? CK_AddressSpaceConversion
+: CK_BitCast);
+} else {
yaxunl wrote:
> pxli168 wrote:
> > I am quite confused by these codes. It seems in some situations you need
> > both BitCast and AddressSpaceConversion.
> > It seems the logic here is too complex. Maybe you can try to simplify it
> >
> if the addr space is different, CK_AddressSpaceConversion is used, which
> corresponds to addrspacecast in LLVM (it is OK if pointee base types are
> different here, addrspacecast covers that, no need for an extra bitcast).
>
> I've tried to simplify the logic. Any suggestion how to further simplify the
> logic?
>
Yes, it's a bit complicated here because we are trying to extend C rules.
In general we might have the following situations:
1. If LHS and RHS types match exactly and:
(a) AS match => use standard C rules, no bitcast or addrspacecast
(b) AS overlap => generate addrspacecast
(c) As don't overlap => give an error
2. if LHS and RHS types don't match:
(a) AS match => use standard C rules, generate bitcast
(b) AS overlap => generate addrspacecast instead of bitcast
(c) AS don't overlap => give an error
I think however we are missing testing all of the cases at the moment. Could
you please add more tests!
Repository:
rL LLVM
http://reviews.llvm.org/D17412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
yaxunl marked 2 inline comments as done.
Comment at: lib/Sema/SemaExpr.cpp:6194-6203
@@ +6193,12 @@
+
+ incompatTy = S.Context.getPointerType(
+ S.Context.getAddrSpaceQualType(S.Context.VoidTy, ResultAddrSpace));
+ LHS = S.ImpCastExprToType(LHS.get(), incompatTy,
+(lhQual.getAddressSpace() != ResultAddrSpace)
+? CK_AddressSpaceConversion
+: CK_BitCast);
+ RHS = S.ImpCastExprToType(RHS.get(), incompatTy,
+(rhQual.getAddressSpace() != ResultAddrSpace)
+? CK_AddressSpaceConversion
+: CK_BitCast);
+} else {
pxli168 wrote:
> I am quite confused by these codes. It seems in some situations you need both
> BitCast and AddressSpaceConversion.
> It seems the logic here is too complex. Maybe you can try to simplify it
>
if the addr space is different, CK_AddressSpaceConversion is used, which
corresponds to addrspacecast in LLVM (it is OK if pointee base types are
different here, addrspacecast covers that, no need for an extra bitcast).
I've tried to simplify the logic. Any suggestion how to further simplify the
logic?
Comment at: lib/Sema/SemaExpr.cpp:6220-6232
@@ -6186,7 +6219,15 @@
ResultTy = S.Context.getBlockPointerType(ResultTy);
- else
+ else {
+auto ResultAddrSpace = ResultTy.getQualifiers().getAddressSpace();
+LHSCastKind = lhQual.getAddressSpace() == ResultAddrSpace
+ ? CK_BitCast
+ : CK_AddressSpaceConversion;
+RHSCastKind = rhQual.getAddressSpace() == ResultAddrSpace
+ ? CK_BitCast
+ : CK_AddressSpaceConversion;
ResultTy = S.Context.getPointerType(ResultTy);
+ }
- LHS = S.ImpCastExprToType(LHS.get(), ResultTy, CK_BitCast);
- RHS = S.ImpCastExprToType(RHS.get(), ResultTy, CK_BitCast);
+ LHS = S.ImpCastExprToType(LHS.get(), ResultTy, LHSCastKind);
+ RHS = S.ImpCastExprToType(RHS.get(), ResultTy, RHSCastKind);
return ResultTy;
pxli168 wrote:
> Why change about block pointer?
> Block can not be used with ternary selection operator (?:) in OpenCL.
This change is for non-block pointer.
For cases
global int* g;
generic int* p;
0?g:p;
Anastasia suggested to change mergeTypes to return type generic int*, then an
implicit cast needs to be inserted for g, which is handled here.
Repository:
rL LLVM
http://reviews.llvm.org/D17412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
pxli168 added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:6194-6203
@@ +6193,12 @@
+
+ incompatTy = S.Context.getPointerType(
+ S.Context.getAddrSpaceQualType(S.Context.VoidTy, ResultAddrSpace));
+ LHS = S.ImpCastExprToType(LHS.get(), incompatTy,
+(lhQual.getAddressSpace() != ResultAddrSpace)
+? CK_AddressSpaceConversion
+: CK_BitCast);
+ RHS = S.ImpCastExprToType(RHS.get(), incompatTy,
+(rhQual.getAddressSpace() != ResultAddrSpace)
+? CK_AddressSpaceConversion
+: CK_BitCast);
+} else {
I am quite confused by these codes. It seems in some situations you need both
BitCast and AddressSpaceConversion.
It seems the logic here is too complex. Maybe you can try to simplify it
Comment at: lib/Sema/SemaExpr.cpp:6220-6232
@@ -6186,7 +6219,15 @@
ResultTy = S.Context.getBlockPointerType(ResultTy);
- else
+ else {
+auto ResultAddrSpace = ResultTy.getQualifiers().getAddressSpace();
+LHSCastKind = lhQual.getAddressSpace() == ResultAddrSpace
+ ? CK_BitCast
+ : CK_AddressSpaceConversion;
+RHSCastKind = rhQual.getAddressSpace() == ResultAddrSpace
+ ? CK_BitCast
+ : CK_AddressSpaceConversion;
ResultTy = S.Context.getPointerType(ResultTy);
+ }
- LHS = S.ImpCastExprToType(LHS.get(), ResultTy, CK_BitCast);
- RHS = S.ImpCastExprToType(RHS.get(), ResultTy, CK_BitCast);
+ LHS = S.ImpCastExprToType(LHS.get(), ResultTy, LHSCastKind);
+ RHS = S.ImpCastExprToType(RHS.get(), ResultTy, RHSCastKind);
return ResultTy;
Why change about block pointer?
Block can not be used with ternary selection operator (?:) in OpenCL.
Repository:
rL LLVM
http://reviews.llvm.org/D17412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
yaxunl updated the summary for this revision.
yaxunl removed a reviewer: pekka.jaaskelainen.
yaxunl added a subscriber: pekka.jaaskelainen.
yaxunl set the repository for this revision to rL LLVM.
yaxunl updated this revision to Diff 48967.
yaxunl marked 5 inline comments as done.
yaxunl added a comment.
Revised as Anastasia suggested.
Modified ASTContext::mergeTypes to handle types with different address space
properly.
Added more sema tests.
Repository:
rL LLVM
http://reviews.llvm.org/D17412
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/ASTContext.cpp
lib/Sema/SemaExpr.cpp
test/CodeGenOpenCL/address-spaces-conversions.cl
test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
Index: test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
===
--- test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
+++ test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
@@ -206,6 +206,38 @@
// expected-error@-2{{arithmetic operation with operands of type ('__constant int *' and '__generic int *') which are pointers to non-overlapping address spaces}}
#endif
+ AS int *var_cond;
+ arg_gen = 0 ? var_cond : arg_glob;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__global int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ arg_gen = 0 ? var_cond : arg_loc;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__local int *') which are pointers to non-overlapping address spaces}}
+#elif defined(GLOBAL)
+// expected-error@-4{{conditional operator with the second and third operands of type ('__global int *' and '__local int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ var_cond = 0 ? var_cond : arg_const;
+#ifdef GENERIC
+// expected-error@-2{{conditional operator with the second and third operands of type ('__generic int *' and '__constant int *') which are pointers to non-overlapping address spaces}}
+#elif defined(GLOBAL)
+// expected-error@-4{{conditional operator with the second and third operands of type ('__global int *' and '__constant int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ arg_gen = 0 ? var_cond : arg_priv;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and 'int *') which are pointers to non-overlapping address spaces}}
+#elif defined(GLOBAL)
+// expected-error@-4{{conditional operator with the second and third operands of type ('__global int *' and 'int *') which are pointers to non-overlapping address spaces}}
+#endif
+
+ arg_gen = 0 ? var_cond : arg_gen;
+#ifdef CONSTANT
+// expected-error@-2{{conditional operator with the second and third operands of type ('__constant int *' and '__generic int *') which are pointers to non-overlapping address spaces}}
+#endif
+
f_glob(var_sub);
#ifndef GLOBAL
// expected-error-re@-2{{passing '__{{constant|generic}} int *' to parameter of type '__global int *' changes address space of pointer}}
Index: test/CodeGenOpenCL/address-spaces-conversions.cl
===
--- test/CodeGenOpenCL/address-spaces-conversions.cl
+++ test/CodeGenOpenCL/address-spaces-conversions.cl
@@ -19,4 +19,6 @@
// CHECK: %{{.*}} = ptrtoint i32 addrspace(1)* %{{.*}} to i64
var_priv = arg_gen > arg_glob; // comparison
// CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(1)* %{{[0-9]+}} to i32 addrspace(4)*
+ arg_gen = arg_glob ? arg_gen : arg_glob; // conditional operator
+ // CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(1)* %{{[0-9]+}} to i32 addrspace(4)*
}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -171,7 +171,6 @@
// Don't do this for enums, they can't be redeclared.
if (isa(D) || isa(D))
break;
-
bool Warn = !AA->isInherited();
// Objective-C method declarations in categories are not modelled as
// redeclarations, so manually look for a redeclaration in a category
@@ -6168,27 +6167,69 @@
QualType CompositeTy = S.Context.mergeTypes(lhptee, rhptee);
if (CompositeTy.isNull()) {
-S.Diag(Loc, diag::ext_typecheck_cond_incompatible_pointers)
- << LHSTy << RHSTy << LHS.get()->getSourceRange()
- << RHS.get()->getSourceRange();
// In this situation, we assume void* type. No especially good
// reason, but this is what gcc does, and we do have to pick
// to get a consistent AST.
-QualType incompatTy = S.Context.getPointerType(S.Context.VoidTy);
-LHS = S.ImpCastExprToType(LHS.get(), incompatTy, CK_BitCast);
-RHS = S.ImpCastExprToType(RHS.get(), incompatTy, CK_BitCast);
+QualType incompatTy;
+if (S.getLangOpts().OpenCL) {
+
