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

[Anastasia Stulova via cfe-commits]

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