[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp:80
+  // Ensure that we still get 3 different template instantiations.
+  tmpl(GLOB);
+  // CHECK-DAG: [[GLOB_LOAD4:%[a-zA-Z0-9]+]] = load i32 addrspace(1)*, i32 addrspace(1)* addrspace(4)* [[GLOB]].ascast
----------------
bader wrote:
> Anastasia wrote:
> > What functionality in the patch does this test?
> > What functionality in the patch does this test?
> 
> As I mentioned in the comment for clang/lib/AST/ItaniumMangle.cpp, there was a problem with instantiating templates for parameters which differs only by address space attribute. Lines 79-91 cover mangling changes.
You are not testing the mangling though?


================
Comment at: clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp:124
+
+template <typename name, typename Func>
+__attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) {
----------------
bader wrote:
> Anastasia wrote:
> > Does this test anything from the patch?
> > Does this test anything from the patch?
> 
> It's the same case as for `address-space-cond-op.cpp` test. This function defines an entry point for the device code. If it's not called, SYCL device compiler won't emit any LLVM IR output. I uploaded the final version of the test, which assumes that SYCL device compiler code generation part is in place. I would prefer to keep it instead of adding it back with my next patch. Is this okay?
If you are not testing any logic from this patch let's leave it out because it is hard to navigate in the large review.


================
Comment at: clang/test/CodeGenSYCL/address-spaces-struct.cpp:1
+// RUN: %clang_cc1 -triple spir64-unknown-linux-sycldevice -fsycl -fsycl-is-device -disable-llvm-passes -emit-llvm -x c++ %s -o - | FileCheck %s
+
----------------
bader wrote:
> Anastasia wrote:
> > What do you test in this file?
> > What do you test in this file?
> 
> This test was added to cover a bug in CodeGen with incompatible address spaces between pointers to a struct and its members.
> As we replaced CodeGen changes with SPIR target callbacks I think it's not needed anymore. I removed this test.
> 
> BTW, the same applies to clang/test/CodeGenSYCL/address-space-cond-op.cpp. Should we remove it as well?
Yeah if it is not testing anything in your patch then it's better to remove it.


================
Comment at: clang/test/SemaSYCL/address-space-parameter-conversions.cpp:25
+
+  bar(*NoAS);
+  bar2(*NoAS);
----------------
bader wrote:
> Anastasia wrote:
> > Btw you don't seem to be testing the reverse conversion i.e. from named to `Default`.
> > Btw you don't seem to be testing the reverse conversion i.e. from named to `Default`.
> 
> Don't `bar2(*GLOB);`, `bar2(*LOC);`, `bar2(*PRIV);` cover this conversion?
Sorry I mean from `Default` to named AS.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909