[PATCH] D138284: Fix incorrect cast in VisitSYCLUniqueStableNameExpr

[Alexander Richardson via Phabricator via cfe-commits]

arichardson 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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138284