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

[Alexey Bader via Phabricator via llvm-commits]

bader added inline comments.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:4554
+
+      // Language rules define if it is legal to cast from one address space
+      // to another, and which address space we should use as a "common
----------------
asavonic wrote:
> bader wrote:
> > asavonic wrote:
> > > sdmitriev wrote:
> > > > bader wrote:
> > > > > Anastasia wrote:
> > > > > > What language rules?
> > > > > I suppose it's a generic note and author didn't meant some particular language here. This change was contributed by @sdmitriev. 
> > > > > @sdmitriev, could you clarify, please?
> > > > This comment was originally added by @asavonic to clang/lib/CodeGen/CGExprScalar.cpp (lines 2962-2965 in this patch), and later I reused his code along with the comment in this file while fixing a very similar issue. So, I guess it would be more correct to ask Andrew to clarify. @asavonic, can you please comment on this?
> > > > This comment was originally added by @asavonic to clang/lib/CodeGen/CGExprScalar.cpp (lines 2962-2965 in this patch), and later I reused his code along with the comment in this file while fixing a very similar issue.
> > > 
> > > There is `if (Opts.SYCLIsDevice)` In CGExprScalar.cpp, so we handle SYCL language rules.
> > > In SYCL, (almost) all pointers have default address space in AST, but in CG some pointers become private/local/global, and some are lowered as generic pointers. We need to add an implicit addrspacecast in CG to emit an expression that expects the same type for its operands.
> > > 
> > > For example:
> > >     const char *str = ... ;
> > >     const char *phi_str = i > 2 ? str : "hello world!";
> > > 
> > > In AST types of `str` and `"hello world"` are the same (`const char *`). In CG we emit `str` as `i8 addrspace(4)*` and `"hello world"` as `i8*`. At this point we have to decide what type `phi_str` should have, and addrspacecast the operands accordingly.
> > Is it possible to avoid fixing address space mismatch at this point by adjusting AST as suggested by John in this comment: https://reviews.llvm.org/D89909#inline-837495?
> > 
> > I wonder if it can be done w/o impacting C++ template metaprogramming as noted in this thread: http://clang-developers.42468.n3.nabble.com/RFC-Re-use-OpenCL-address-space-attributes-for-SYCL-td4068754.html.
> > Is it possible to avoid fixing address space mismatch at this point by adjusting AST
> 
> Some address space mismatches are present in CG already, see CodeGenModule::getStringLiteralAddressSpace and CodeGenModule::GetGlobalVarAddressSpace. This should be moved to AST (or replaced) as well, right?
> Some address space mismatches are present in CG already, see CodeGenModule::getStringLiteralAddressSpace and CodeGenModule::GetGlobalVarAddressSpace. This should be moved to AST (or replaced) as well, right?

Kind of. If I'm not mistaken the idea is to redefine some TargetInfo hooks for SPIR target (similar to AMDGPU target) and IRGen should be able to align address spaces without additional customization like this. John provided a little bit more details in the RFC thread: http://clang-developers.42468.n3.nabble.com/RFC-Re-use-OpenCL-address-space-attributes-for-SYCL-td4068754.html#a4069039.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909