[Mesa-dev] [PATCH v2 00/22] Introducing SPIR-V support to clover
Karol Herbst
kherbst at redhat.com
Tue Jan 23 23:03:11 UTC 2018
On Tue, Jan 23, 2018 at 11:46 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> Pierre Moreau <pierre.morrow at free.fr> writes:
>
>> On 2018-01-23 — 14:02, Francisco Jerez wrote:
>>> 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.
>>
>> “On top of” SPIR-V, not “instead of”, as SPIR-V is the only IL which is
>> mandatory to support, according to the specification.
>
> That's right, but it just means that devices that have LLVM as canonical
> IR don't get support for cl_khr_il_program for the time being, until
> Khronos' SPIRV-to-LLVM converter gets upstreamed.
>
we could use tomeus out of tree llvm-spirv module though, but this
would also need some maintenance. It would be a better solution than
using that llvm-spirv fork from khronos
>> But I completely agree with having one canonical IR to deal with inside clover,
>> be it SPIR-V or LLVM IR, until an executable is generated.
>>
>>>
>>> >> * 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?).
>>
>> It is missing from this series. As it only came up very recently (as in one or
>> two days ago), and we were not certain which way to go yet, I have kept the
>> series as-is, with plan to fix it depending on what the feedback was.
>>
>> I am not sure whether glGetProgramInfo(CL_PROGRAM_IL) is supposed to return the
>> exact original IL given through clCreateProgramWithIL, or if it can return a
>> modified version of it. If it can be modified, then we could use LLVM IR as the
>> canonical IR, and just convert back to SPIR-V when someone asks for the
>> program’s IL.
>>
>
> I don't see any requirement for it to be unmodified in the extension
> spec. That said this is probably not a problem because LLVM-IR-friendly
> pipe drivers are not going to be able to support this extension until
> Khronos' converter becomes available upstream, at which point we will
> also have a converter to go back from LLVM IR to SPIR-V if that turns
> out to be a requirement.
>
>>>
>>> > 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
>>> >>
More information about the mesa-dev
mailing list