[Mesa-dev] [PATCH] mesa: define nir_spirv_supported_capabilities
Alejandro Piñeiro
apinheiro at igalia.com
Thu Dec 7 08:17:20 UTC 2017
On 07/12/17 06:23, Jason Ekstrand wrote:
> On Wed, Dec 6, 2017 at 8:48 PM, Timothy Arceri <tarceri at itsqueeze.com
> <mailto:tarceri at 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 <tarceri at itsqueeze.com
> <mailto:tarceri at 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 at lists.freedesktop.org
> <mailto:mesa-dev at 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 at lists.freedesktop.org
> <mailto:mesa-dev at 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 at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
>
More information about the mesa-dev
mailing list