[Mesa-dev] [PATCH v2 00/22] Introducing SPIR-V support to clover
Francisco Jerez
currojerez at riseup.net
Tue Jan 23 22:46:03 UTC 2018
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.
> 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
>> >>
-------------- 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/fab6a104/attachment-0001.sig>
More information about the mesa-dev
mailing list