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

[Yaxun Liu via cfe-commits]

yaxunl marked 15 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())
----------------
pxli168 wrote:
> 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.
Here if unqualified types are different or CVS qualifiers are different, the two types cannot be merged, so an empty QualType is returned.

If unqualified types and CVS qualifiers are the same, we check if address spaces are compatible and returns the merged type.

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

================
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;
----------------
Anastasia wrote:
> There are still blocks in ObjC, we shouldn't change!
line 6218 handles the blocks

================
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:
> 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.
ImpCastExprToType handles the case when the target type matches the value, it simply does not generate the cast.

================
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);
----------------
Anastasia wrote:
> Btw, could we do the same thing with {LHS/RHS}CastKind variables in abode code. It looks cleaner I think.
Sorry what is abode code?

================
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
 
----------------
it seems to be separate issue. I will take a look in another patch


Repository:
  rL LLVM

http://reviews.llvm.org/D17412