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

Thu Feb 8 20:47:55 UTC 2018

Pierre Moreau <pierre.morrow at free.fr> writes:

> On 2018-02-07 — 12:36, Francisco Jerez wrote:
>> Pierre Moreau <pierre.morrow at free.fr> writes:
>> 
>> > 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).
>> >
>> 
>> I thought we had agreed to continue supporting the current "clc->llvm
>> ir->native binary->driver" path for targets that have an LLVM back-end
>> (e.g. AMDGPU), so we can just give the pipe driver a binary, and also
>> allow the application to query and cache programs as native machine
>> code?
>
> Sorry, I must have misunderstood you then. I thought we had agreed to use
> either SPIR-V or LLVM IR as the “canonical” IR, i.e. we would compile to and
> link using that IR, and then convert to whatever IR the driver supports. So for
> example, we compile to SPIR-V, link the different SPIR-V modules together, and
> if we are creating an executable, we convert the linked module to native
> machine code and store that in the clover module under the executable section.
> That way, the driver receives the native code, and when querying for binaries,
> the native code is returned (or whatever IR the driver supports).
> For libraries, we want to keep them in SPIR-V/LLVM IR, as they will be used in
> a linking step at some point.
>

I don't think you misunderstood, but it seemed so from your previous
paragraph. ;)

>> >> 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).
>> >
>> 
>> Maybe that the translation is unnecessary and is going to create more
>> than zero work for the pipe driver to obtain a binary?
>
> If the translation is unnecessary (I am talking about the translation happening
> on the executable, before it is passed to the driver), then that IR should be
> part of PIPE_SHADER_CAP_SUPPORTED_IRS.
> Unless we decide to go with using LLVM IR in some cases and SPIR-V in other
> cases, akin to what we have currently between LLVM IR and TGSI, there will
> always be some overhead:

Yes, there's always going to be overhead from multiple conversions in
some cases (e.g. SPIR-V input on a driver that wants a native binary
ultimately), but ideally the responsibility of converting back and forth
would be handled by the state tracker instead of by the driver.

> either converting a SPIR-V input to LLVM IR to later go back via
> SPIR-V to NIR, or go via LLVM IR to SPIR-V before going back to LLVM
> IR.

Why would we ever have to do either of those?  If the driver wants
SPIR-V or NIR as IR, a SPIR-V input doesn't need to be translated to
LLVM IR in the first place.  Likewise if the driver wants a native
binary for an input in CL C, it doesn't need to be converted to SPIR-V
in the first place.

> I thought the plan was to either use LLVM IR or SPIR-V for everyone (as all current
> consumers, and future ones via NIR, can be handled from those two IRs), but if
> that’s not the case, then I am fine with that as well.
>
>> >> > 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.
>> >
>> 
>> As long as we use PIPE_SHADER_CAP_SUPPORTED_IRS to enumerate the IRs the
>> driver is willing to accept I don't think there is a real conflict, the
>> pipe driver can just set both bits and have the state tracker choose
>> which one to provide.
>
> Sure.
>
>> >> 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.
>> >
>> 
>> Yeah, I think having a spir-v target-(un)specific implementation of
>> libclc would be a pretty reasonable plan.
>
> I’ll most likely need some help/pointers on that, but I’ll do that in a
> separate thread.
>
>> > 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.
>> >
>> 
>> I don't think constant specialization changes the picture at all, AFAIUI
>> they only apply to kernels that are written in SPIR-V in the first
>> place, and require a rebuild to take effect, so we could handle them in
>> exactly the same way regardless of whether the SPIR-V is being
>> translated into LLVM IR during compilation or whether it's being used as
>> program representation internally by clover.
>
> I had forgotten binaries fed to clCreateProgramWithIL() would only be
> converted, if necessary, during the compilation phase, so indeed the constant
> specialisation has no impact.
>
> Regards,
> Pierre
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180208/0081bd0f/attachment.sig>