[Mesa-dev] [PATCH 11/11] r600/radeonsi/clover: always assume PIPE_SHADER_IR_NATIVE for clover

[Pierre Moreau]

On 2018-02-06 — 20:50, Jan Vesely wrote:
[snip]
> > > Happy to here suggestions for solving the current conflict in uses of
> > > PIPE_SHADER_CAP_PREFERRED_IR.
> > 
> > One option could be to:
> > * look at the preferred IR
> >   |-> if clover supports it, use it
> >   |-> else, check if any IR supported by clover are supported by the driver,
> >       and pick the first one that works
> > 
> > Also, clover will be switching (experimentally first) to using SPIR-V as the
> > canonical IR, for all the compiling and linking internally, before translating
> > the resulting executable to a representation the driver can handle.
> 
> Why? what is this good for? Is the expected path for radeonsi:
>  clc->llvm->spirv->nir->llvm->isa, or clc->llvm->spirv->llvm->isa ?

It depends how we end up deciding which IR to feed the driver (using
*_PREFERRED_IR, *_SUPPORTED_IRS, or something else), but the path would be
clc->llvm->spirv->*->driver or spirv->*->driver depending on how the programs
are created (resp. clCreateProgramWithSource or clCreateProgramWithIL).

> What advantages does it bring to any target? why can't the targets that
> are not natively supported by llvm just consume llvm IR and do the
> necessary translations on driver side?

What disadvantages does it bring to have clover take care of the translation
(for IRs that are easily translatable from LLVM IR/SPIR-V)? It already has all
the dependencies needed, the translation is implemented in a single common
place, and if you don’t want OpenCL you do not need to still depend on LLVM
(except for the drivers for AMD cards).

> > So once
> > spirv_to_nir supports OpenCL SPIR-V (which I believe is being worked on by Rob
> > Clark for freedreno, and Karol Herbst for Nouveau), RadeonSI could start
> > accepting even OpenCL kernels as NIR.
> 
> Why would anyone want that?

In case it simplifies the code in the driver; I haven’t looked at that code
though. It was only a suggestion, and meant that for RadeonSI, there could no
longer be a conflict between the IR fed from OpenCL and the one from OpenGL,
which was one of the problem addressed in this patch.

> what is the expected path from CLC? if you
> need the external spirv tools package, why not use it in the other
> direction and keep LLVM IR as clover preferred representation?
> note that libclc is distributed in target specific LLVM IR.

I am not entirely sure what the interaction with libclc should be, as I am only
starting to look into it. Possibly we would add a spir[64] target to it and use
that.

We could indeed use LLVM IR instead of SPIR-V, as mentioned by Francisco Jerez
in his reply to “[PATCH v2 00/22] Introducing SPIR-V support to clover” on the
23rd of January 2018 (I don’t have a direct link as this is one of the emails
which did not get archived), which shouldn’t change much apart from having:

* clc->(compile)->llvm->(link)->llvm->(translate to driver IR)
* spirv->llvm->(link)->llvm->(translate to driver IR)

instead of

* clc->(compile)->spirv->(link)->spirv->(translate to driver IR)
* spirv->(link)->spirv->(translate to driver IR)

(For drivers using NIR, the translation would be either llvm->spirv->nir or
spirv->nir; I don’t think there is a llvm->nir pass, but I could be mistaken.)

However, with OpenCL 2.2 you can specialise constants, so if we use LLVM IR, we
would either need to
1. keep a hash map from SPIR-V IDs to LLVM IR variables/functions and edit the
   LLVM module;
2. convert back to SPIR-V (and hope that the SPIR-V IDs didn’t change),
   specialise, and convert back again to LLVM IR.
Or, if we use SPIR-V, we just do the specialisation directly.

Also, OpenCL C++ is only supported through clCreateProgramWithIL(), which will
give us a SPIR-V binary.

Regards,
Pierre
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180207/303a8b51/attachment.sig>