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

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

I think it's hard to write this simpler as logic is quite complicated. But we can definitely improve understanding with comments!

Btw, regarding tests to cover case 2, could we also add test with types in which CV qualifiers differ. May be just at least one. No need to test every possible combination.


================
Comment at: lib/AST/ASTContext.cpp:7613
@@ +7612,3 @@
+    if (getLangOpts().OpenCL) {
+      if (LHS.getUnqualifiedType() != RHS.getUnqualifiedType() ||
+          LQuals.getCVRQualifiers() != RQuals.getCVRQualifiers())
----------------
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.

================
Comment at: lib/Sema/SemaExpr.cpp:6168
@@ -6168,3 +6167,3 @@
   QualType CompositeTy = S.Context.mergeTypes(lhptee, rhptee);
 
   if (CompositeTy.isNull()) {
----------------
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.

================
Comment at: lib/Sema/SemaExpr.cpp:6175
@@ +6174,3 @@
+    if (S.getLangOpts().OpenCL) {
+      // OpenCL v1.1 s6.5 - A pointer to address space A can only be assigned
+      // to a pointer to the same address space A. Casting a pointer to address
----------------
Could we write it shorter? For example: Conversion between pointers to distinct address spaces is disallowed...


================
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;
----------------
There are still blocks in ObjC, we shouldn't change!

================
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);
----------------
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!

================
Comment at: lib/Sema/SemaExpr.cpp:6231
@@ -6189,3 +6230,3 @@
 
-  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);
----------------
Btw, could we do the same thing with {LHS/RHS}CastKind variables in abode code. It looks cleaner I think.

================
Comment at: test/SemaOpenCL/condition-operator-cl2.0.cl:19
@@ +18,3 @@
+
+void test_conversion(global int *arg_glob, local int *arg_loc,
+                     constant int *arg_const, private int *arg_priv,
----------------
May be we could rename it to test_ternary and also merge with test/SemaOpenCL/address-spaces-conversions-cl2.0.cl?

================
Comment at: test/SemaOpenCL/condition-operator-cl2.0.cl:20
@@ +19,3 @@
+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,
----------------
Too many arguments. Could we move some into local variables instead?

================
Comment at: test/SemaOpenCL/condition-operator-cl2.0.cl:33
@@ +32,3 @@
+#ifdef CONSTANT
+// expected-error at -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)
----------------
You can combine these two errors into 1 using regex. See examples in function test_conversion of test/SemaOpenCL/address-spaces-conversions-cl2.0.cl

================
Comment at: test/SemaOpenCL/condition-operator-cl2.0.cl:57
@@ +56,3 @@
+
+  void *var_void_gen;
+  constant void*var_void_const;
----------------
Could you declare just before the line using it?


Repository:
  rL LLVM

http://reviews.llvm.org/D17412