Re: [Mesa-dev] [PATCH v2 05/25] mesa: implement SPIR-V loading in glShaderBinary

2017-12-13 Thread Jon Turney 

On 30/11/2017 23:57, Ian Romanick wrote:

On 11/30/2017 09:28 AM, Eduardo Lima Mitev wrote:

From: Nicolai Hähnle 

v2: * Add a gl_shader_spirv_data member to gl_shader, which already
encapsulates a gl_spirv_module where the binary will be saved.
(Eduardo Lima)

 * Just use the 'spirv_data' member to know whether a gl_shader has
the SPIR_V_BINARY_ARB state. (Timothy Arceri)

 * Remove redundant argument checks. Move extension presence check
to API entry point where the rest of checks are. Retype 'n' and
'length'arguments to use the correct and more standard types.
(Ian Romanick)
---
  src/mesa/main/glspirv.c   | 43 +++
  src/mesa/main/glspirv.h   |  5 +
  src/mesa/main/mtypes.h|  4 
  src/mesa/main/shaderapi.c | 45 ++---
  src/mesa/main/shaderobj.c |  2 ++
  5 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c
index 8d1e652e088..d2e76bb1927 100644
--- a/src/mesa/main/glspirv.c
+++ b/src/mesa/main/glspirv.c

[...]

+
+   sh = alloca(sizeof(*sh) * (size_t)n);
+   if (!sh) {
+  _mesa_error(ctx, GL_OUT_OF_MEMORY, "glShaderBinary");
+  return;
+   }
+


This adds a use of alloca() without a corresponding #include.

Patch attached.

From 3b00c72a92ca1091d11ecffd8db404dcd598e63d Mon Sep 17 00:00:00 2001
From: Jon Turney 
Date: Wed, 13 Dec 2017 19:49:07 +
Subject: [PATCH] Fix use of alloca() without #include 
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

../../../src/mesa/main/shaderapi.c: In function ‘_mesa_ShaderBinary’:
../../../src/mesa/main/shaderapi.c:2188:9: error: implicit declaration of 
function ‘alloca’ [-Werror=implicit-function-declaration]

Signed-off-by: Jon Turney 
---
 src/mesa/main/shaderapi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index d824a88ca2f..2c11e4d5bb6 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -38,6 +38,7 @@
 
 
 #include 
+#include 
 #include "main/glheader.h"
 #include "main/context.h"
 #include "main/dispatch.h"
-- 
2.15.1

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

Re: [Mesa-dev] [PATCH v2 05/25] mesa: implement SPIR-V loading in glShaderBinary

2017-11-30 Thread Ian Romanick 
Two nits below...

On 11/30/2017 09:28 AM, Eduardo Lima Mitev wrote:
> From: Nicolai Hähnle 
> 
> v2: * Add a gl_shader_spirv_data member to gl_shader, which already
>encapsulates a gl_spirv_module where the binary will be saved.
>(Eduardo Lima)
> 
> * Just use the 'spirv_data' member to know whether a gl_shader has
>the SPIR_V_BINARY_ARB state. (Timothy Arceri)
> 
> * Remove redundant argument checks. Move extension presence check
>to API entry point where the rest of checks are. Retype 'n' and
>'length'arguments to use the correct and more standard types.
>(Ian Romanick)
> ---
>  src/mesa/main/glspirv.c   | 43 +++
>  src/mesa/main/glspirv.h   |  5 +
>  src/mesa/main/mtypes.h|  4 
>  src/mesa/main/shaderapi.c | 45 ++---
>  src/mesa/main/shaderobj.c |  2 ++
>  5 files changed, 96 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c
> index 8d1e652e088..d2e76bb1927 100644
> --- a/src/mesa/main/glspirv.c
> +++ b/src/mesa/main/glspirv.c
> @@ -25,6 +25,8 @@
>  
>  #include "errors.h"
>  
> +#include "errors.h"
> +
>  #include "util/u_atomic.h"
>  
>  void
> @@ -59,6 +61,47 @@ _mesa_shader_spirv_data_reference(struct 
> gl_shader_spirv_data **dest,
>p_atomic_inc(>RefCount);
>  }
>  
> +void
> +_mesa_spirv_shader_binary(struct gl_context *ctx,
> +  unsigned n, struct gl_shader **shaders,
> +  const void* binary, size_t length)
> +{
> +   struct gl_spirv_module *module;
> +   struct gl_shader_spirv_data *spirv_data;
> +
> +   assert(length >= 0);
> +
> +   module = malloc(sizeof(*module) + (size_t)length);

Don't need the (size_t) because you made length be size_t. :)

> +   if (!module) {
> +  _mesa_error(ctx, GL_OUT_OF_MEMORY, "glShaderBinary");
> +  return;
> +   }
> +
> +   p_atomic_set(>RefCount, 0);
> +   module->Length = length;
> +   memcpy(>Binary[0], binary, length);
> +
> +   for (int i = 0; i < n; ++i) {
> +  struct gl_shader *sh = shaders[i];
> +
> +  spirv_data = rzalloc(NULL, struct gl_shader_spirv_data);
> +  _mesa_shader_spirv_data_reference(>spirv_data, spirv_data);
> +  _mesa_spirv_module_reference(_data->SpirVModule, module);
> +
> +  sh->CompileStatus = compile_failure;
> +
> +  free((void *)sh->Source);
> +  sh->Source = NULL;
> +  free((void *)sh->FallbackSource);
> +  sh->FallbackSource = NULL;
> +
> +  ralloc_free(sh->ir);
> +  sh->ir = NULL;
> +  ralloc_free(sh->symbols);
> +  sh->symbols = NULL;
> +   }
> +}
> +
>  void GLAPIENTRY
>  _mesa_SpecializeShaderARB(GLuint shader,
>const GLchar *pEntryPoint,
> diff --git a/src/mesa/main/glspirv.h b/src/mesa/main/glspirv.h
> index b8a0125ea9f..ba281f68bef 100644
> --- a/src/mesa/main/glspirv.h
> +++ b/src/mesa/main/glspirv.h
> @@ -71,6 +71,11 @@ void
>  _mesa_shader_spirv_data_reference(struct gl_shader_spirv_data **dest,
>struct gl_shader_spirv_data *src);
>  
> +void
> +_mesa_spirv_shader_binary(struct gl_context *ctx,
> +  unsigned n, struct gl_shader **shaders,
> +  const void* binary, size_t length);
> +
>  /**
>   * \name API functions
>   */
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 062eea609c7..50a47e0a65d 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -98,6 +98,7 @@ struct st_context;
>  struct gl_uniform_storage;
>  struct prog_instruction;
>  struct gl_program_parameter_list;
> +struct gl_shader_spirv_data;
>  struct set;
>  struct set_entry;
>  struct vbo_context;
> @@ -2646,6 +2647,9 @@ struct gl_shader
> GLuint TransformFeedbackBufferStride[MAX_FEEDBACK_BUFFERS];
>  
> struct gl_shader_info info;
> +
> +   /* ARB_gl_spirv related data */
> +   struct gl_shader_spirv_data *spirv_data;
>  };
>  
>  
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 72824355838..24058e5ee2e 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -42,6 +42,7 @@
>  #include "main/context.h"
>  #include "main/dispatch.h"
>  #include "main/enums.h"
> +#include "main/glspirv.h"
>  #include "main/hash.h"
>  #include "main/mtypes.h"
>  #include "main/pipelineobj.h"
> @@ -1051,6 +1052,16 @@ set_shader_source(struct gl_shader *sh, const GLchar 
> *source)
>  {
> assert(sh);
>  
> +   /* The GL_ARB_gl_spirv spec adds the following to the end of the 
> description
> +* of ShaderSource:
> +*
> +*   "If  was previously associated with a SPIR-V module (via the
> +*ShaderBinary command), that association is broken. Upon successful
> +*completion of this command the SPIR_V_BINARY_ARB state of 
> +*is set to FALSE."
> +*/
> +   _mesa_shader_spirv_data_reference(>spirv_data, NULL);
> +
> if

[Mesa-dev] [PATCH v2 05/25] mesa: implement SPIR-V loading in glShaderBinary

2017-11-30 Thread Eduardo Lima Mitev 
From: Nicolai Hähnle 

v2: * Add a gl_shader_spirv_data member to gl_shader, which already
   encapsulates a gl_spirv_module where the binary will be saved.
   (Eduardo Lima)

* Just use the 'spirv_data' member to know whether a gl_shader has
   the SPIR_V_BINARY_ARB state. (Timothy Arceri)

* Remove redundant argument checks. Move extension presence check
   to API entry point where the rest of checks are. Retype 'n' and
   'length'arguments to use the correct and more standard types.
   (Ian Romanick)
---
 src/mesa/main/glspirv.c   | 43 +++
 src/mesa/main/glspirv.h   |  5 +
 src/mesa/main/mtypes.h|  4 
 src/mesa/main/shaderapi.c | 45 ++---
 src/mesa/main/shaderobj.c |  2 ++
 5 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c
index 8d1e652e088..d2e76bb1927 100644
--- a/src/mesa/main/glspirv.c
+++ b/src/mesa/main/glspirv.c
@@ -25,6 +25,8 @@
 
 #include "errors.h"
 
+#include "errors.h"
+
 #include "util/u_atomic.h"
 
 void
@@ -59,6 +61,47 @@ _mesa_shader_spirv_data_reference(struct 
gl_shader_spirv_data **dest,
   p_atomic_inc(>RefCount);
 }
 
+void
+_mesa_spirv_shader_binary(struct gl_context *ctx,
+  unsigned n, struct gl_shader **shaders,
+  const void* binary, size_t length)
+{
+   struct gl_spirv_module *module;
+   struct gl_shader_spirv_data *spirv_data;
+
+   assert(length >= 0);
+
+   module = malloc(sizeof(*module) + (size_t)length);
+   if (!module) {
+  _mesa_error(ctx, GL_OUT_OF_MEMORY, "glShaderBinary");
+  return;
+   }
+
+   p_atomic_set(>RefCount, 0);
+   module->Length = length;
+   memcpy(>Binary[0], binary, length);
+
+   for (int i = 0; i < n; ++i) {
+  struct gl_shader *sh = shaders[i];
+
+  spirv_data = rzalloc(NULL, struct gl_shader_spirv_data);
+  _mesa_shader_spirv_data_reference(>spirv_data, spirv_data);
+  _mesa_spirv_module_reference(_data->SpirVModule, module);
+
+  sh->CompileStatus = compile_failure;
+
+  free((void *)sh->Source);
+  sh->Source = NULL;
+  free((void *)sh->FallbackSource);
+  sh->FallbackSource = NULL;
+
+  ralloc_free(sh->ir);
+  sh->ir = NULL;
+  ralloc_free(sh->symbols);
+  sh->symbols = NULL;
+   }
+}
+
 void GLAPIENTRY
 _mesa_SpecializeShaderARB(GLuint shader,
   const GLchar *pEntryPoint,
diff --git a/src/mesa/main/glspirv.h b/src/mesa/main/glspirv.h
index b8a0125ea9f..ba281f68bef 100644
--- a/src/mesa/main/glspirv.h
+++ b/src/mesa/main/glspirv.h
@@ -71,6 +71,11 @@ void
 _mesa_shader_spirv_data_reference(struct gl_shader_spirv_data **dest,
   struct gl_shader_spirv_data *src);
 
+void
+_mesa_spirv_shader_binary(struct gl_context *ctx,
+  unsigned n, struct gl_shader **shaders,
+  const void* binary, size_t length);
+
 /**
  * \name API functions
  */
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 062eea609c7..50a47e0a65d 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -98,6 +98,7 @@ struct st_context;
 struct gl_uniform_storage;
 struct prog_instruction;
 struct gl_program_parameter_list;
+struct gl_shader_spirv_data;
 struct set;
 struct set_entry;
 struct vbo_context;
@@ -2646,6 +2647,9 @@ struct gl_shader
GLuint TransformFeedbackBufferStride[MAX_FEEDBACK_BUFFERS];
 
struct gl_shader_info info;
+
+   /* ARB_gl_spirv related data */
+   struct gl_shader_spirv_data *spirv_data;
 };
 
 
diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 72824355838..24058e5ee2e 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -42,6 +42,7 @@
 #include "main/context.h"
 #include "main/dispatch.h"
 #include "main/enums.h"
+#include "main/glspirv.h"
 #include "main/hash.h"
 #include "main/mtypes.h"
 #include "main/pipelineobj.h"
@@ -1051,6 +1052,16 @@ set_shader_source(struct gl_shader *sh, const GLchar 
*source)
 {
assert(sh);
 
+   /* The GL_ARB_gl_spirv spec adds the following to the end of the description
+* of ShaderSource:
+*
+*   "If  was previously associated with a SPIR-V module (via the
+*ShaderBinary command), that association is broken. Upon successful
+*completion of this command the SPIR_V_BINARY_ARB state of 
+*is set to FALSE."
+*/
+   _mesa_shader_spirv_data_reference(>spirv_data, NULL);
+
if (sh->CompileStatus == compile_skipped && !sh->FallbackSource) {
   /* If shader was previously compiled back-up the source in case of cache
* fallback.
@@ -2132,9 +2143,7 @@ _mesa_ShaderBinary(GLint n, const GLuint* shaders, GLenum 
binaryformat,
const void* binary, GLint length)
 {
GET_CURRENT_CONTEXT(ctx);
-   (void) shaders;
-   (void) binaryformat;
-   (void) binary;
+   struct gl_shader **sh;