[PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator

[Anastasia Stulova via cfe-commits]

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