[OpenCL] Generate bitcast when target address space does not change.

[yaxunl]

Summary

In codegen different address spaces may be mapped to the same address
space for a target, e.g. in x86/x86-64 all address spaces are mapped
to 0.

The following OpenCL C code

global int *a;
generic void *b = a;

will generate AST like

`-ImplicitCastExpr '__generic void *' <AddressSpaceConversion>
   `-ImplicitCastExpr '__global int *' <LValueToRValue>
     `-DeclRefExpr '__global int *' lvalue Var 0x195157005e0 'a' '__global int *'

In codegen, both __generic and __global address spaces are mapped to 0.
If AddressSpaceConversion is mapped to addrspacecast, assertion will happen
since the source and target address spaces are the same.

The fix is to use CreatePointerBitCastOrAddrSpaceCast for translating
AddressSpaceConversion.

Differential Revision: http://reviews.llvm.org/D18713

[rjmccall]

lib/CodeGen/CGExprScalar.cpp

1421 ↗

(On Diff #52423)

This is just CreatePointerBitCastOrAddrSpaceCast.

[yaxunl]

Use CreatePointerBitCastOrAddrSpaceCast as John suggested.

[rjmccall]

LGTM, thanks!

[Anastasia]

LG, apart from renaming the test file.

Btw, there is another place in lib/CodeGen/CGExprScalar.cpp with CreateAddrSpaceCast call too. I am wondering if that could have the same issue... if yes, may be we should fix it already now.

test/CodeGenOpenCL/2016-04-01-addrcast.cl

1 ↗

(On Diff #52431)

Can we rename the file to just contain the name i.e. something like addrspacecast.cl would be fine.

Actually, we also have similar functionality in test/CodeGenOpenCL/address-spaces-conversions.cl. Do you think it might make sense to integrate? Let's say to have two RUN lines with and w/o -ffake-address-space-map.

[yaxunl]

Btw, there is another place in lib/CodeGen/CGExprScalar.cpp with CreateAddrSpaceCast call too. I am wondering if that could have the same issue... if yes, may be we should fix it already now.

I cannot find another CreateAddrSpaceCast in lib/CodeGen/CGExprScalar.cpp with the latest commit 885217218dd21498430d50c5a2f6dd8e329add4b. Do you have the commit hash and line number?

Thanks.

[yaxunl]

test/CodeGenOpenCL/2016-04-01-addrcast.cl

1 ↗

(On Diff #52431)

I think it is a good idea to add a run w/o -ffake-address-space-map to address-spaces-conversions.cl for better coverage.

[yaxunl]

Removed 2016-04-01-addrcast.cl and it to address-spaces-conversions.cl.

[Anastasia]

In D18713#391406, @yaxunl wrote:

Btw, there is another place in lib/CodeGen/CGExprScalar.cpp with CreateAddrSpaceCast call too. I am wondering if that could have the same issue... if yes, may be we should fix it already now.

I cannot find another CreateAddrSpaceCast in lib/CodeGen/CGExprScalar.cpp with the latest commit 885217218dd21498430d50c5a2f6dd8e329add4b. Do you have the commit hash and line number?

Thanks.

If I check online I can still see it on line 1414: http://clang.llvm.org/doxygen/CGExprScalar_8cpp_source.html

[yaxunl]

If I check online I can still see it on line 1414: http://clang.llvm.org/doxygen/CGExprScalar_8cpp_source.html

This is the only place CreateAddrSpaceCast showing up and being fixed by this patch.

[Anastasia]

test/CodeGenOpenCL/address-spaces-conversions.cl

11 ↗

(On Diff #52607)

Would it make sense to check here and below for NOFAKE too? Basically to check that there is no addrspacecast generated?

[yaxunl]

Add more checks for w/o -ffake-address-space-map to the test.

[Anastasia]

LGTM! Thanks!

cfe/trunk/lib/CodeGen/CGExprScalar.cpp

if (CGF.SanOpts.has(SanitizerKind::CFIUnrelatedCast)) {
                                       CodeGenFunction::CFITCK_UnrelatedCast,
                                       CE->getLocStart());
     }
 
     return Builder.CreateBitCast(Src, DstTy);
   }
   case CK_AddressSpaceConversion: {
     Value *Src = Visit(const_cast<Expr*>(E));
-    return Builder.CreateAddrSpaceCast(Src, ConvertType(DestTy));
+    // Since target may map different address spaces in AST to the same address
+    // space, an address space conversion may end up as a bitcast.
+    return Builder.CreatePointerBitCastOrAddrSpaceCast(Src,
+                                                       ConvertType(DestTy));
   }
   case CK_AtomicToNonAtomic:
   case CK_NonAtomicToAtomic:
   case CK_NoOp:
   case CK_UserDefinedConversion:
     return Visit(const_cast<Expr*>(E));
 
   case CK_BaseToDerived: {

cfe/trunk/test/CodeGenOpenCL/address-spaces-conversions.cl

 // RUN: %clang_cc1 %s -triple x86_64-unknown-linux-gnu -O0 -ffake-address-space-map -cl-std=CL2.0 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple x86_64-unknown-linux-gnu -O0 -cl-std=CL2.0 -emit-llvm -o - | FileCheck --check-prefix=CHECK-NOFAKE %s
+// When -ffake-address-space-map is not used, all addr space mapped to 0 for x86_64.
 
 // test that we generate address space casts everywhere we need conversions of
 // pointers to different address spaces
 
 void test(global int *arg_glob, generic int *arg_gen) {
   int var_priv;
   arg_gen = arg_glob; // implicit cast global -> generic
   // CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(1)* %{{[0-9]+}} to i32 addrspace(4)*
+  // CHECK-NOFAKE-NOT: addrspacecast
 
   arg_gen = &var_priv; // implicit cast with obtaining adr, private -> generic
   // CHECK: %{{[0-9]+}} = addrspacecast i32* %var_priv to i32 addrspace(4)*
+  // CHECK-NOFAKE-NOT: addrspacecast
+
   arg_glob = (global int *)arg_gen; // explicit cast
   // CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(4)* %{{[0-9]+}} to i32 addrspace(1)*
+  // CHECK-NOFAKE-NOT: addrspacecast
+
   global int *var_glob =
       (global int *)arg_glob; // explicit cast in the same address space
   // CHECK-NOT: %{{[0-9]+}} = addrspacecast i32 addrspace(1)* %{{[0-9]+}} to i32 addrspace(1)*
+  // CHECK-NOFAKE-NOT: addrspacecast
+
   var_priv = arg_gen - arg_glob; // arithmetic operation
   // CHECK: %{{.*}} = ptrtoint i32 addrspace(4)* %{{.*}} to i64
   // CHECK: %{{.*}} = ptrtoint i32 addrspace(1)* %{{.*}} to i64
+  // CHECK-NOFAKE: %{{.*}} = ptrtoint i32* %{{.*}} to i64
+  // CHECK-NOFAKE: %{{.*}} = ptrtoint i32* %{{.*}} to i64
+
   var_priv = arg_gen > arg_glob; // comparison
   // CHECK: %{{[0-9]+}} = addrspacecast i32 addrspace(1)* %{{[0-9]+}} to i32 addrspace(4)*
+
+  generic void *var_gen_v = arg_glob;
+  // CHECK: addrspacecast
+  // CHECK-NOT: bitcast
+  // CHECK-NOFAKE: bitcast
+  // CHECK-NOFAKE-NOT: addrspacecast
 }