On Tue, Jan 23, 2018 at 11:46 PM, Francisco Jerez <[email protected]> wrote: > Pierre Moreau <[email protected]> writes: > >> On 2018-01-23 — 14:02, Francisco Jerez wrote: >>> Karol Herbst <[email protected]> writes: >>> >>> > there seem to be some patches missing? >>> > >>> > On Tue, Jan 23, 2018 at 1:33 AM, Pierre Moreau <[email protected]> >>> > 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 >>> >> _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
