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

[Yaxun Liu via cfe-commits]

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