[PATCH] D54947: [OpenCL][CodeGen] Fix replacing memcpy with addrspacecast

Wed Nov 28 10:17:24 PST 2018

sidorovd added a comment.

@yaxunl, since I'm partially reverting your change https://reviews.llvm.org/D34367 can you give a feedback on this?

================
Comment at: lib/CodeGen/CGCall.cpp:3972
+          // we don't want to perform address space cast for it, since that
+          // leads to casting __private * (default addr space in OpenCL) to
+          // __global * which is not valid. Create memcpy call instead.
----------------
Anastasia wrote:
> This statement is incorrect. Private is only default AS in CL versions before 2.0.
> 
> I feel this can be expressed simpler. I guess the observation here is that if addr space mismatches it has to generate a copy because even if it would be a generic we can't get what the original specific addr space was? So it's safe to generate a copy.
> This statement is incorrect. Private is only default AS in CL versions before 2.0.
You are right, thanks!

Still I want to leave the comment almost as is, since it creates a link with a previous one on lines 3928-3935.

================
Comment at: test/CodeGenOpenCL/addr-space-struct-arg.cl:4
 // RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 -finclude-default-header -triple amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL1.2 -O0 -finclude-default-header -triple spir-unknown-unknown-unknown | FileCheck -enable-var-scope -check-prefixes=SPIR %s

----------------
Anastasia wrote:
> Do you know why we need `-finclude-default-header` here? If it's just for vector types perhaps we should just declare them here... headers parsing is expensive in terms of time.
Yeap, it's just for vectors. Added typedef for int2.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54947/new/

https://reviews.llvm.org/D54947