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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
