Pierre Moreau <pierre.mor...@free.fr> writes: > On 2018-01-23 — 14:02, Francisco Jerez wrote: >> Karol Herbst <kher...@redhat.com> writes: >> >> > there seem to be some patches missing? >> > >> > On Tue, Jan 23, 2018 at 1:33 AM, Pierre Moreau <pierre.mor...@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 >> >>
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
