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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits