[PATCH] D109818: [HIPSPV] Convert HIP kernels to SPIR-V kernels

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Sep 22 02:34:24 PDT 2021


Anastasia added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10221
+ABIArgInfo SPIRABIInfo::classifyKernelArgumentType(QualType Ty) const {
+  if (getContext().getLangOpts().HIP && getTarget().getTriple().isSPIRV()) {
+    // Coerce pointer arguments with default address space to CrossWorkGroup
----------------
linjamaki wrote:
> Anastasia wrote:
> > It feels like this needs to be in `SPIRVABIInfo`  or something? Or can this be generalized to both  - SPIR and SPIR-V?
> A comment was added in D109144 to state that the SPIRABIInfo is an ABI implementation for both the SPIR and SPIR-V. For now, there is not much difference between SPIR and SPIR-V for this class. Would it be satisfactory if the class is renamed to something more general (like CommonSPIRABIInfo)?
It might be reasonable to rename indeed or we can just amend its documentation to clarify what it is used for both...

However if you need to specialize different logic I think the conventional way in clang would be to create a subclass...

This is related to a wider discussion that is ongoing about the best way to reuse SPIR for SPIR-V and their evolution... CCing @bader here too.


================
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);
----------------
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109818



More information about the cfe-commits mailing list