Re: [Mesa-dev] [PATCH] mesa: define nir_spirv_supported_capabilities

[Alejandro Piñeiro]

On 07/12/17 06:23, Jason Ekstrand wrote:
> On Wed, Dec 6, 2017 at 8:48 PM, Timothy Arceri  > 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
 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  >
>
>
>
> 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 :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
> 
> 
>
>   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
>

Re: [Mesa-dev] [PATCH] mesa: define nir_spirv_supported_capabilities

[Jason Ekstrand]

On Wed, Dec 6, 2017 at 8:48 PM, Timothy Arceri 
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.

--Jason


> Reviewed-by: Timothy Arceri 
>
>
>
>> 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: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-Novembe
 r/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

 

Re: [Mesa-dev] [PATCH] mesa: define nir_spirv_supported_capabilities

[Timothy Arceri]

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:


Reviewed-by: Timothy Arceri 




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: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

  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
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


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa: define nir_spirv_supported_capabilities

[Ian Romanick]

Reviewed-by: Ian Romanick 

On 12/06/2017 03:57 AM, 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
> 
>  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
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa: define nir_spirv_supported_capabilities

[Ian Romanick]

On 12/06/2017 07:29 AM, 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.
> 
> 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?

I expect that over time there will be overlap between SPIR-V
functionality exposed in OpenCL, Vulkan, and OpenGL extensions.  There
already are some cases of this.  Given that, I think having a single
master list of supported capabilities makes sense.

> 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.
> 
> Regards,
> Pierre
> 
> On 2017-12-06 — 09: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
>>
>>  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
>> 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




signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa: define nir_spirv_supported_capabilities

[Alejandro Piñeiro]

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 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: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
>>
>>  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
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev




signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa: define nir_spirv_supported_capabilities

[Pierre Moreau]

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.

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?

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.

Regards,
Pierre

On 2017-12-06 — 09: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
> 
>  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
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev