[PATCH] D138284: Fix incorrect cast in VisitSYCLUniqueStableNameExpr

[Alexey Bader via Phabricator via cfe-commits]

bader added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1635
+      Context.getTargetInfo().getConstantAddressSpace().value_or(
+          LangAS::Default));
   llvm::Constant *GlobalConstStr = Builder.CreateGlobalStringPtr(
----------------
arichardson wrote:
> arichardson wrote:
> > 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)`?
> Ah it looks like we should be using `CodeGenModule::GetGlobalConstantAddressSpace()` instead of `getTarget().getConstantAddressSpace()`. Is that correct?
> 
> 
> ```
> LangAS CodeGenModule::GetGlobalConstantAddressSpace() const {
>   // OpenCL v1.2 s6.5.3: a string literal is in the constant address space.
>   if (LangOpts.OpenCL)
>     return LangAS::opencl_constant;
>   if (LangOpts.SYCLIsDevice)
>     return LangAS::sycl_global;
>   if (LangOpts.HIP && LangOpts.CUDAIsDevice && getTriple().isSPIRV())
>     // For HIPSPV map literals to cuda_device (maps to CrossWorkGroup in SPIR-V)
>     // instead of default AS (maps to Generic in SPIR-V). Otherwise, we end up
>     // with OpVariable instructions with Generic storage class which is not
>     // allowed (SPIR-V V1.6 s3.42.8). Also, mapping literals to SPIR-V
>     // UniformConstant storage class is not viable as pointers to it may not be
>     // casted to Generic pointers which are used to model HIP's "flat" pointers.
>     return LangAS::cuda_device;
>   if (auto AS = getTarget().getConstantAddressSpace())
>     return *AS;
>   return LangAS::Default;
> }
> ```
> 
> Another problem appears to be that the default implementation of getConstantAddressSpace() returns `LangAS::Default` instead of None, so the .value_or() will never be used.
> Ah it looks like we should be using CodeGenModule::GetGlobalConstantAddressSpace() instead of getTarget().getConstantAddressSpace(). Is that correct?

Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138284