[Mesa-dev] [PATCH v2 00/22] Introducing SPIR-V support to clover

Tue Jan 23 22:02:32 UTC 2018

Karol Herbst <kherbst at redhat.com> writes:

> there seem to be some patches missing?
>
> On Tue, Jan 23, 2018 at 1:33 AM, Pierre Moreau <pierre.morrow at free.fr> wrote:
>> Hello,
>>
>> Here is the second version of my initial series for adding SPIR-V support to
>> clover, after the RFC back in May 2017.
>>
>> For recap, the focus of this series is to let clover accept SPIR-V binaries
>> through the cl_khr_il_program extension (from OpenCL 1.2 and on), as well as
>> through some core features (since OpenCL 2.1). Even if OpenCL 2.1 support in
>> clover is some way off, there is another motivation for supporting SPIR-V in
>> clover, as multiple drivers are interested in adding OpenCL support by
>> converting SPIR-V to NIR.
>>
>> Note: the series is based on master + Karol’s patch “clover: add functions up
>> to 2.2 to ICD dispatch table”.
>>
>>
>> The various patches can be split in different categories:
>>
>> * Patches 1 through 7: some clover clean-up, adding and moving some
>>   functionalities around to make the implementation easier in the rest of the
>>   series.
>>
>> * Patches 8 through 13: define SPIR-V as a new IR, add a new frontend to clover
>>   to deal with SPIR-V, and edit compile and link operations to handle SPIR-V as
>>   well.
>>
>> * Patches 14 through 19: implement cl_khr_il_program
>>
>> * Patches 20 through 22: implement OpenCL 2.1 support on top of
>>   cl_khr_il_program
>>
>>
>> Changes since the RFC
>> ---------------------
>>
>> * Most SPIR-V utilities were dropped, and the remaining ones have been moved to
>>   the clover SPIR-V frontend rather than sitting in src/gallium/auxiliary/spirv.
>>
>> * The SPIR-V linker has been completely dropped from this series and instead
>>   merge in SPIRV-Tools [1].
>>
>> * Since SPIRV-Tools now exports a pkgconfig .pc file, use it for detecting the
>>   library.
>>
>> * Integrate the series with Meson.
>>
>> * Use pipe_llvm_program_header to pass in the size of the SPIR-V module, rather
>>   than adding a new attribute to pipe_compute_state, as suggested by Francisco
>>   Jerez.
>>
>> * Check that the device supports the capabilities defined in the SPIR-V binary.
>>
>> * Check that the platform/device supports the extensions used in the SPIR-V
>>   binary.
>>
>> * Fix the implementation responsible for filling up the symbols of the clover
>>   module based on the input SPIR-V binary.
>>
>> * No longer raw SPIR-V binaries through clCreateProgramWithBinary, but instead
>>   keep the current de-serialisation of the clover module, which may contain a
>>   SPIR-V binary.
>>
>> * Track whether a library was created with the --enable-link-options flag or
>>   not. This is currently not useful as the linker ignores most link options,
>>   but it will become useful when the linker handles those options.
>>
>> * Implement cl_khr_il_program.
>>
>> * Most of patches 1 through 8 (apart from patch 2).
>>
>>
>> Discussions
>> -----------
>>
>> * Before, when linking different modules together, you knew that all modules
>>   would use the same IR, as all were created using clCreateProgramWithSource,
>>   therefore the linker could just call the linking function corresponding to
>>   the target’s preferred IR. But with the introduction of
>>   clCreateProgramWithIL(KHR)?, we can now end up in a case where we try to link
>>   a module using NIR as IR (created through clCreateProgramWithSource, assuming
>>   that is the driver’s preferred IR), with another module using SPIR-V as IR
>>   (created through clCreateProgramWithIL). How do we handle such a case: should
>>   we translate the SPIR-V to NIR and use a NIR linker on them, or convert NIR
>>   to SPIR-V and use the SPIR-V linker? NIR and LLVM IR can be handled
>>   relatively easily, but what about TGSI?
>>
>
> I think we will never be able to convert all IRs into any other IR, so
> that I would suggest to leave those IRs unconverted until they get
> linked together and there the code can decide on a common IR for
> linking. So if we get source code, we can parse it to llvm IR and
> leave it like that until it gets linked. Converting back and forth
> would require us to write all those conversion paths and I am assume
> this wouldn't be worth the trouble.
>

I think it would be more straightforward to compile source programs into
SPIRV if the driver supports it (or if it supports any other IR that
could possibly be translated from SPIRV after link time, e.g. NIR or
maybe even TGSI).  That means that there is a single canonical IR for
each CL device and we don't need to deal with linking different
combinations of IRs together.  If the driver doesn't support SPIRV nor
any of the IRs derived from it, it better support LLVM IR instead, so we
can just use that as canonical IR within the state tracker, and possibly
accept the same representation as input to clCreateProgramWithIL()
instead of SPIRV.

>> * In that regard, is anyone using the TGSI frontend in clover? If not, is
>>   anyone planning to use it? And if still not, shouldn’t we just remove it?
>>
>> * In the same vein as the linking discussion just above, what should happen
>>   when the driver’s preferred IR is one of the IRs not currently supported by
>>   clover, like NIR for example? Should `compile()` generate a SPIR-V binary
>>   which is directly translated to NIR, or should we keep everything in SPIR-V
>>   until the very last moment, right before sending the IR to the driver?  If
>>   all the drivers supporting compute through clover support an IR that can be
>>   translated from SPIR-V, it might be easier to keep everything inside clover
>>   as SPIR-V binaries, until we need to pass the program to the driver, in which
>>   case we convert it on the fly.
>>
>
> don't rely on the preferred IR inside clover. It should only matter
> _after_ linking. If we can't link to the preferred IR, but to a
> supported one, that's fine as well. I am sure that over the time where
> we actually start supporting multiple IRs we would run into troubles
> if we strictly rely on the preferred one.

I agree that SPIRV kernels should probably be kept around as SPIRV until
link time, because linking can be done on SPIRV, and the result can then
be translated to whatever IR the driver is asking for (I believe this
last point is what is missing from this series?).

> Or we have to do some fundamental changes on how to figure out the
> preferred IR, like a driver wants to prefer something else if it is a
> OpenCL shader instead of an OpenGL compute shader?
>

I'd argue that the driver should be as agnostic as possible what compute
API is sitting on top.  Ideally drivers wouldn't need to implement a
separate IR translation pass for each API they intend to support.

> Also using SPIR-V as the common IR might be not what the AMD driver
> wants to do, because they just want to get straight to LLVM -> native.
>
>>
>> (Still) missing
>> ---------------
>>
>> * As there is no upstream version of LLVM which can produce SPIR-V out of
>>   OpenCL code, clCreateProgramWithSource will refuse to work if the target’s
>>   preferred IR is SPIR-V, for now.
>>
>
> here again, please don't rely too much on the preferred IR. Most
> drivers will either have NIR, LLVM or TGSI set there. So to force
> SPIR-V you are already out of luck and introduce some level of pain
> for drivers to actually handle this situation. In general we need a
> better way to figure out what IR to actually use for the driver.
>
> In the end we should never fail to compile/link just because we can't
> produce the preferred IR. If the driver can't handle any of the
> supported IRs the driver either have to fix it or stop exposing
> support for it.
>
>> * Optimisation linking options are ignored for now as SPIRV-Tools’ linker does
>>   not supported them yet.
>>
>>
>> Thank you in advance for reviewing/commenting,
>> Pierre
>>
>> [1]: https://github.com/KhronosGroup/SPIRV-Tools/
>>
>>
>> Pierre Moreau (22):
>>   clover/api: Fix tab indentation to spaces
>>   clover: Add additional functions to query supported IRs
>>   clover/api: Fail if trying to build a non-executable binary
>>   clover: Disallow creating libraries from other libraries
>>   clover: Track flags per module section
>>   clover: Move device extensions definitions to core/device.cpp
>>   clover: Move platform extensions definitions to clover/platform.cpp
>>   include/pipe: Define SPIRV as an IR
>>   configure.ac,meson: Check for SPIRV-Tools
>>   clover/spirv: Import spirv.hpp11 version 1.0 (rev 12)
>>   clover/spirv: Add functions for parsing arguments, linking programs,
>>     etc.
>>   clover: Refuse to compile source code to SPIR-V
>>   clover: Handle the case when linking SPIR-V binaries together
>>   clover: Add a pointer property to return ILs
>>   include/CL: Add cl_khr_il_program
>>   clover: Implement clCreateProgramWithILKHR
>>   clover: Handle CL_PROGRAM_IL_KHR in clGetProgramInfo
>>   clover/api: Implement CL_DEVICE_IL_VERSION_KHR
>>   clover: Advertise cl_khr_il_program
>>   include/CL: Export OpenCL 2.1 functions
>>   clover: Implement clCreateProgramWithIL from OpenCL 2.1
>>   clover: Use OpenCL 2.1 defines in place of cl_khr_il_program
>>
>>  configure.ac                                       |   5 +
>>  include/CL/cl.h                                    |   8 +
>>  include/CL/cl_ext.h                                |  34 +
>>  include/CL/cl_platform.h                           |   1 +
>>  meson.build                                        |   2 +
>>  src/gallium/include/pipe/p_defines.h               |   1 +
>>  src/gallium/state_trackers/clover/Makefile.am      |  15 +-
>>  src/gallium/state_trackers/clover/Makefile.sources |   4 +
>>  src/gallium/state_trackers/clover/api/device.cpp   |  16 +-
>>  src/gallium/state_trackers/clover/api/dispatch.cpp |   2 +-
>>  src/gallium/state_trackers/clover/api/dispatch.hpp |   4 +
>>  src/gallium/state_trackers/clover/api/platform.cpp |   6 +-
>>  src/gallium/state_trackers/clover/api/program.cpp  |  70 +-
>>  src/gallium/state_trackers/clover/core/device.cpp  |  26 +
>>  src/gallium/state_trackers/clover/core/device.hpp  |   4 +
>>  src/gallium/state_trackers/clover/core/module.cpp  |   1 +
>>  src/gallium/state_trackers/clover/core/module.hpp  |  13 +-
>>  .../state_trackers/clover/core/platform.cpp        |   5 +
>>  .../state_trackers/clover/core/platform.hpp        |   2 +
>>  src/gallium/state_trackers/clover/core/program.cpp |  94 +-
>>  src/gallium/state_trackers/clover/core/program.hpp |  14 +
>>  .../state_trackers/clover/core/property.hpp        |  39 +
>>  .../state_trackers/clover/llvm/codegen/bitcode.cpp |   3 +-
>>  .../state_trackers/clover/llvm/codegen/common.cpp  |   2 +-
>>  src/gallium/state_trackers/clover/meson.build      |  10 +-
>>  .../state_trackers/clover/spirv/invocation.cpp     | 668 ++++++++++++++
>>  .../state_trackers/clover/spirv/invocation.hpp     |  54 ++
>>  .../state_trackers/clover/spirv/spirv.hpp11        | 997 +++++++++++++++++++++
>>  .../state_trackers/clover/tgsi/compiler.cpp        |   3 +-
>>  29 files changed, 2067 insertions(+), 36 deletions(-)
>>  create mode 100644 src/gallium/state_trackers/clover/spirv/invocation.cpp
>>  create mode 100644 src/gallium/state_trackers/clover/spirv/invocation.hpp
>>  create mode 100644 src/gallium/state_trackers/clover/spirv/spirv.hpp11
>>
>> --
>> 2.16.0
>>
-------------- 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/20180123/6f61f663/attachment-0001.sig>