[PATCH] D108621: [HIPSPV] Add CUDA->SPIR-V address space mapping

[Henry Linjamäki via Phabricator via llvm-commits]

linjamaki added inline comments.


================
Comment at: clang/lib/Basic/Targets/SPIR.h:233
+    if (Opts.HIP && Opts.CUDAIsDevice)
+      // Enable address space mapping from HIP to SPIR-V.
+      // See comment on the SPIRDefIsGenMap table.
----------------
Anastasia wrote:
> linjamaki wrote:
> > Anastasia wrote:
> > > My guess is that this is not only HIP specific but for example the same applies to SYCL.
> > > 
> > > I am not sure if it makes more sense to move this into a `BaseSPIRTargetInfo` since this is not really SPIR-V specific logic. It is just a clang design misalignment between two address space concepts that has to be addressed properly at some point.
> > > 
> > The DefaultIsGeneric AS mapping is enabled for SYCL in the BaseSPIRTargetInfo::adjust (which also means the mapping is available for both the SPIR and SPIR-V targets). On the other hand, the AS mapping for HIPSPV is enabled in SPIRVTargetInfo::adjust only as we intend to emit SPIR-V only. I’m under the impression that this is what was wanted.
> I think the issues here is not related to the target but to the flaw in the address space design in clang. So right now all languages that don't derive the address space semantic from embedded C (SYCL/CUDA/HIP) would need to reset the address space map. See FIXME comment in the definition of `adjust`.
> 
> So the right thing to do would be to set the address space map correctly straight away based on the language being compiled for which would avoid overriding this in `adjust`. But if we do override it then it makes more sense to at least unify the logic among targets.
> 
> 
> > 
> > On the other hand, the AS mapping for HIPSPV is enabled in SPIRVTargetInfo::adjust only as we intend to emit SPIR-V only. 
> > 
> 
> 
> I am not really sure how you would support one target only.
> Clang architecture (at least originally) assumes that all languages can map to all targets which in practice is not true in some cases. This is why we need to provide an address space map even for targets that have no memory segmented language compiled to it. 

> So the right thing to do would be to set the address space map correctly straight away based on the language being compiled for which would avoid overriding this in `adjust`. But if we do override it then it makes more sense to at least unify the logic among targets.
> 

Since we are not sure how we would solve this issue optimally, we adjusted the patch to avoid adding more overrides for the `adjust` method and the logic previously in the `SPIRVTargetInfo::adjust` is moved to `BaseSPIRTargetInfo::adjust` with the SYCL.  Would this be sufficient for the functionality added by this patch?

> I am not really sure how you would support one target only.
> Clang architecture (at least originally) assumes that all languages can map to all targets which in practice is not true in some cases. This is why we need to provide an address space map even for targets that have no memory segmented language compiled to it. 

“HIPSPV” is not meant to be a new language. We are just adjusting the address space mapping from the HIP language (for device code) to SPIR-V that suits better than the default mapping where all the HIP address spaces would be mapped to target address space zero. We map the address spaces to the suitable ones in the OpenCL standard, which works both for HIPCL (which uses the OpenCL-based runtime and the OpenCL SPIR-V profile) and HIPLZ (which uses the LZ-based runtime and also the OpenCL SPIR-V profile).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108621