[PATCH] D110622: [HIPSPV][3/4] Enable SPIR-V emission for HIP

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In D110622#3174113 <https://reviews.llvm.org/D110622#3174113>, @tra wrote:

> The patch looks OK for the time being. That said, I do have concerns that we may be organically growing something that will be troublesome to deal with long-term.
>
> TBH, I still can't quite make sense of where/how SPIR-V fits in the offloading nomenclature.
>
> Right now we have multiple levels of offloading-related control points.
>
> - offload targets, specified by --offload-arch. Determines the ISA of the GPU binary we produce.
> - offload mechanism: OpenMP, CUDA runtime, HSA. Determines how we compile/pack/launch the GPU binaries.
> - front-end: CUDA/HIP/ C/C++ w/ OpenMP.
> - Driver: Determines compilation pipeline to glue everything together,
>
> SPIR-V in these patches appears to be wearing multiple hats. 
> It changes compilation pipeline, it changes offload mechanism and it changes offload targets.

>From my POV, SPIR-V is "the ISA of GPU binary we produce" and we might need some changes at offloading-related control points:

- offload mechanism: none of listed "offload mechanisms" (i.e. OpenMP, CUDA runtime, HSA) is able to handle SPIR-V natively. On the other hand, I'm not sure if there is a need in additional changes for all "offloading mechanisms". E.g. Intel's compiler implements OpenMP-offload to SPIR-V target using OpenMP runtime plug-in lowering OpenMP runtime calls to OpenCL/Level Zero. OpenCL and Level Zero <https://spec.oneapi.io/level-zero/latest/index.html> runtimes are able to compile and launch SPIR-V binaries.
- front-end: if we compare SPIR to other ISAs, they change compilation pipeline as well (e.g. add new built-ins to expose ISA, add CodeGen library changes to emit ISA specific metadata, etc.). AMDGPU ISA <https://llvm.org/docs/AMDGPUUsage.html#llvm> or NVIDIA <https://docs.nvidia.com/cuda/nvvm-ir-spec/index.html> GPU <https://llvm.org/docs/NVPTXUsage.html> ISA changed front-end/compilation pipeline as well. Do you refer to some other non-ISA specific changes? BTW, shameless plug, the patch adding built-in functions and types for SPIR-V ISA is under review here - https://reviews.llvm.org/D108034.
- Driver: front-end compiler doesn't support SPIR-V format yet (i.e. SPIR-V requires special encoding different from LLVM bitcode) , so Driver hooks up LLVM->SPIR-V translator tool to produce SPIR-V binary.

> To further complicate things, it appears to be a derivative of the HIP compilation. I can't tell if it's an implementation detail at the moment, or whether it will become a more generic offload mechanism that would be expected to be used by other front- and back-ends. E.g. can we potentially compile CUDA code to target SPIR-V? Can OpenMP offload to SPIR-V?

Intel's compiler compiles OpenMP offload and SYCL to SPIR-V, so we definitely would like to target SPIR-V using other front-ends.

> So, the question is -- what's the right way to specify something like this in a consistent manner? 
> `--offload` option proposed here does not seem to be a good fit. It was intended as a more flexible way to create a single `-cc1` sub-compilation and we're doing quite a bit more here.

Does `--offload-arch=spirv*` fit better here? If I understand the goal of this patch correctly, it tries to provide controls for changing offload target for HIP application from default (AMDGCN) to SPIR-V.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110622