[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

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

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

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!

lib/CodeGen/CGExprScalar.cpp

                                       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: {
     const CXXRecordDecl *DerivedClassDecl = DestTy->getPointeeCXXRecordDecl();
     assert(DerivedClassDecl && "BaseToDerived arg isn't a C++ object pointer!");
 
     Address Base = CGF.EmitPointerWithAlignment(E);

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

+// RUN: %clang_cc1 %s -triple x86_64-unknown-linux-gnu -O0 -cl-std=CL2.0 -emit-llvm -o - | FileCheck %s
+
+// When -ffake-address-space-map is not used, all address space mapped to 0 for x86/x86-64, a bitcast should be generated instead of addrspacecast.
+void foo() {
+  global int *a;
+  generic void *b = a;
+  // CHECK: bitcast 
+  // CHECK-NOT: addrspacecast
+}