[PATCH] D138284: Fix incorrect cast in VisitSYCLUniqueStableNameExpr

Fri Nov 18 07:21:59 PST 2022

arichardson added a comment.

In D138284#3936846 <https://reviews.llvm.org/D138284#3936846>, @erichkeane wrote:

> In D138284#3936830 <https://reviews.llvm.org/D138284#3936830>, @aaron.ballman wrote:
>
>> Adding @bader as SYCL code owner. The changes look reasonable to me, but codegen is not my area of expertise.
>
> Yeah, same to me.  I wrote this originally, but I think the address-space stuff was added in the downstream by someone else (though I can't find proof of it now!).

(Unfortunately) that was me. Not the best design, but it catches lots of subtle run-time errors at compile time. Has been extremely useful for the downstream CHERI fork.

================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1635
+      Context.getTargetInfo().getConstantAddressSpace().value_or(
+          LangAS::Default));
   llvm::Constant *GlobalConstStr = Builder.CreateGlobalStringPtr(
----------------
bader wrote:
> > This changes the code generation for spir64 to place the globals in addrspace(4). I believe is correct, but it would be good for someone who is familiar with the target to confirm.
> 
> Globals must reside in `sycl_global` namespace, which is `addrspace(1)` for spir* targets.
> `addrspace(4)` represents "generic" address space, which is a placeholder for a specific address space. If we leave it `addrspace(4)` for global definition, the compiler won't be able to infer genuine address space.
Okay that's interesting - I guess it means we should not be using `getConstantAddressSpace()` here? Or getConstantAddressSpace() should actually return a value that maps to `addrspace(1)`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138284