linjamaki added inline comments.
================ Comment at: clang/lib/CodeGen/TargetInfo.cpp:10224 + // pointers for HIPSPV. When the language mode is HIP, the SPIRTargetInfo + // maps cuda_device to SPIR-V's CrossWorkGroup address space. + llvm::Type *LTy = CGT.ConvertType(Ty); ---------------- bader wrote: > Anastasia wrote: > > linjamaki wrote: > > > Anastasia wrote: > > > > Can you explain why this mapping is needed? We already have an address > > > > space map to perform the mapping of address spaces b/w language and > > > > target. It would be good if we don't replicate similar logic in too > > > > many places. > > > HIP does not require address space qualifiers on kernel pointer arguments > > > (e.g. see hipspv-kernel.cpp) nor there are AS qualifiers that can be > > > placed on them. With the default logic, provided by SPIRVTargetInfo’s > > > address space map, the kernel pointer arguments get converted to generic > > > pointers which are not allowed by the OpenCL SPIR-V Environment > > > Specification. > > I feel that it is the same for SYCL... It might be good to check with > > @bader whether there is already a way to handle this that can be reused for > > HIP... > We need to do similar transformation for SYCL, but it's not exactly the same. > For SYCL kernels, which represented as function objects, compiler generates > SPIR kernel function and fixes up the address space for pointer arguments in > compiler generated declaration. For more details, see the description of > https://reviews.llvm.org/D71016 and `handlePointerType` function code in > clang/lib/Sema/SemaSYCL.cpp of this review request (lines 848-876). As > address space is fixed in Sema, it works for all targets SYCL currently > supports SPIR, NVPTX and AMDGPU. > > If I understand it correctly, we are trying to do minimal amount of work for > convert HIP kernel function to SPIR kernel function, i.e. fix calling > convention and address spaces. > Are these two fixes enough or we need more fixes to enable more sophisticated > kernels? This patch and D108621 covers only the calling convention and the address space aspects in HIP->SPIR-V conversion. There are still various HIP features [1] which need to be expanded or emulated afterwards. A fully linked device program is needed before the fixes can be applied, so these fixes won’t be implemented at Sema nor CodeGen which operate per translation unit. The full-program fixes are applied by the HIPSPV tool chain by applying LLVM passes provided by a HIPSPV runtime [2]. For the time being, we are not seeing a need to specialize SPIR target infos further. [1]: “HIP code expansion” section of the “[RFC][HIPSPV] Emitting HIP device code as SPIR-V” [2]: clang/lib/Driver/ToolChains/HIPSPV.cpp:constructEmitSpirvCommand() @ https://github.com/parmance/llvm-project/tree/hipspv-wip Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109818/new/ https://reviews.llvm.org/D109818 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
