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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/include/clang/AST/Type.h:488
            (A == LangAS::opencl_global && (B == LangAS::opencl_global_device ||
                                            B == LangAS::opencl_global_host)) ||
            // Consider pointer size address spaces to be equivalent to default.
----------------
bader wrote:
> BTW, we need enable `global_device` and `global_host` attributes from https://reviews.llvm.org/D82174 for SYCL USM feature. I have following question regarding this: should I create a follow-up patch or we can enable all attributes for SYCL at once?
It seems like they would just be extending the existing functionality and not redesigning what we do in this patch?

If that's the case let's keep it in a separate patch, but feel free to upload it even now.


================
Comment at: clang/lib/Basic/Targets/SPIR.h:129
+    TargetInfo::adjust(Opts);
+    setAddressSpaceMap(/*DefaultIsGeneric=*/Opts.SYCLIsDevice);
+  }
----------------
Anastasia wrote:
> Ok, do you still plan to add a `FIXME` as mentioned previously explaining why we need to reset the map here?
Btw I was just thinking of another alternative here.

What do you think about just setting a value in `Default` AS entry then we wouldn't need any extra map at all in this case? So something like:


```
AddrSpaceMap[LangAS::Default] = AddrSpaceMap[LangAS::opencl_generic];
```
with a good explanation in `FIXME`? :)



================
Comment at: clang/test/CodeGenSYCL/convergent.cpp:2
 // RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64-unknown-unknown-sycldevice -emit-llvm %s -o - | \
 // RUN:   FileCheck %s
----------------
bader wrote:
> Anastasia wrote:
> > Is this change related? I thought we are not adding the environment component after all...
> > 
> >  
> > Is this change related? I thought we are not adding the environment component after all...
> 
> While I was removing `-sycldevice` environment component from the patch, I noticed that one of the committed tests already uses it.
> https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenSYCL/convergent.cpp#L2
> 
> Do you want to me to create a separate review request for this change?
I see, this looks trivial enough test clean up. You could just commit it without any review perhaps?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909