On 07/12/17 06:23, Jason Ekstrand wrote: > On Wed, Dec 6, 2017 at 8:48 PM, Timothy Arceri <tarc...@itsqueeze.com > <mailto:tarc...@itsqueeze.com>> wrote: > > On 07/12/17 00:23, Alejandro Piñeiro wrote: > > On 06/12/17 13:29, Pierre Moreau wrote: > > Hello Alejandro, > > As far as I understand, nir_spirv_supported_capabilities > is being filled in by > the driver and then fetched by the API entrypoint to check > the capabilities > required by the SPIR-V binary given as input. And this is > done regardless of > the input IR used by the driver, be it NIR, LLVM IR, TGSI > or others. So > couldn’t it be just named spirv_supported_capabilities? > Unless it also reflects > the capabilities supported by the IR being used. > > > Good point. spirv_supported_capabilities is probably a better > name, > although right now, it would be only used on the spirv to nir > pass. I > will not send a new version of the patch with just the > renaming, but for > anyone interested on review the commit, I will use that name. > > > I would be much happier with this being in mtypes.h with that > name. So if renamed: > > > Ugh... I just now got around to looking at this and saw that it went > in mtypes.h. Can we please move it? We've worked very hard to keep > the Vulkan driver from having to pull in any GL headers and data > structures and now a fairly core piece lives in mtypes.h.
Hmm, sorry for not waiting for more feedback. This is the second reviewed patch that modify mtypes.h, so I assumed that it was fine. Additionally, I didn't rename it as Ian's review didn't ask to, but both Timothy and Pierre prefer a more general "spirv_supported_capabilities". So how about this?: * Rename it to spirv_supported_capabilities * Move it to compiler/spirv/spirv.h (would need to add #include <stdbool.h> due the booleans on the struct there) * Add a "compiler/spirv/spirv.h" include on mtypes.h I already have the patches locally, so if you agree with this, it is just about sending them. > > --Jason > > > Reviewed-by: Timothy Arceri <tarc...@itsqueeze.com > <mailto:tarc...@itsqueeze.com>> > > > > I guess nir_spirv_supported_capabilities could be extended > later on to also add > capabilities specific to OpenCL when clover reaches OpenCL > 1.2 support (and can > start accepting SPIR-V binaries as input through the > cl_khr_il_program > extension), or would it be better to have a separate one > for OpenCL? > > > Probably it would be re-used, but I don't know the specifics > of OpenCL > to ensure 100% that. > > > I haven’t had time to look at the whole gl_spirv series > yet, so I am sorry if > this is something that has already been brought and > answered in that thread. > > > No sorries, your feedback was good and welcomed. > > > Regards, > Pierre > > On 2017-12-06 — 09 <tel:2017-12-06%20%E2%80%94%2009>:57, > Alejandro Piñeiro wrote: > > Until now it was part of spirv_to_nir_options. But it > will be used on > the implementation of ARB_gl_spirv and > ARB_spirv_extensions, and added > to the OpenGL context, as a way to save what SPIR-V > capabilities the > current OpenGL implementation supports. > --- > > We are sending this commit in advance of a v3 of the > initial gl_spirv > and spirv_extensions support series. The issue is that > lately there > were a lot of activity on the spirv/spir_to_nir code > base, and we are > being fixing rebase conflicts constantly. Getting this > commit on > master would make things easier. > > FWIW, this patch is similar to one that Ian Romanick > already granted > Rb, but that was dropped after all the mentioned changes: > > https://lists.freedesktop.org/archives/mesa-dev/2017-November/178261.html > > <https://lists.freedesktop.org/archives/mesa-dev/2017-November/178261.html> > > src/compiler/spirv/nir_spirv.h | 16 +++------------- > src/mesa/main/mtypes.h | 12 ++++++++++++ > 2 files changed, 15 insertions(+), 13 deletions(-) > > diff --git a/src/compiler/spirv/nir_spirv.h > b/src/compiler/spirv/nir_spirv.h > index 43ec19d5a50..113bd710a00 100644 > --- a/src/compiler/spirv/nir_spirv.h > +++ b/src/compiler/spirv/nir_spirv.h > @@ -28,7 +28,8 @@ > #ifndef _NIR_SPIRV_H_ > #define _NIR_SPIRV_H_ > -#include "nir/nir.h" > +#include "compiler/nir/nir.h" > +#include "main/mtypes.h" > #ifdef __cplusplus > extern "C" { > @@ -57,18 +58,7 @@ struct spirv_to_nir_options { > */ > bool lower_workgroup_access_to_offsets; > - struct { > - bool float64; > - bool image_ms_array; > - bool tessellation; > - bool draw_parameters; > - bool image_read_without_format; > - bool image_write_without_format; > - bool int64; > - bool multiview; > - bool variable_pointers; > - bool storage_16bit; > - } caps; > + struct nir_spirv_supported_capabilities caps; > struct { > void (*func)(void *private_data, > diff --git a/src/mesa/main/mtypes.h > b/src/mesa/main/mtypes.h > index b478f6158e2..7da05aa3ee9 100644 > --- a/src/mesa/main/mtypes.h > +++ b/src/mesa/main/mtypes.h > @@ -3579,6 +3579,18 @@ struct gl_program_constants > GLuint MaxShaderStorageBlocks; > }; > +struct nir_spirv_supported_capabilities { > + bool float64; > + bool image_ms_array; > + bool tessellation; > + bool draw_parameters; > + bool image_read_without_format; > + bool image_write_without_format; > + bool int64; > + bool multiview; > + bool variable_pointers; > + bool storage_16bit; > +}; > /** > * Constants which may be overridden by device > driver during context creation > -- > 2.11.0 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > <mailto:mesa-dev@lists.freedesktop.org> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > <https://lists.freedesktop.org/mailman/listinfo/mesa-dev> > > > > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > <mailto:mesa-dev@lists.freedesktop.org> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > <https://lists.freedesktop.org/mailman/listinfo/mesa-dev> > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > <https://lists.freedesktop.org/mailman/listinfo/mesa-dev> > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
