Re: [Mesa-dev] [PATCH] glsl/linker: validate attribute aliasing before optimizations

2018-10-29 Thread Tapani Pälli 



On 10/27/18 1:32 AM, Timothy Arceri wrote:

On Fri, Oct 12, 2018, at 5:04 AM, Tapani Pälli wrote:

Patch does a 'dry run' of assign_attribute_or_color_locations before
optimizations to catch cases where we have aliasing of unused attributes
which is forbidden by the GLSL ES 3.x specifications.

We need to run this pass before unused attributes may be removed and with
attribute binding information from program, therefore we re-use existing
pass in linker rather than attempt to write another one.

This fixes WebGL2 test 'gl-bindAttribLocation-aliasing-inactive' and
Piglit test 'gles-3.0-attribute-aliasing'.

Signed-off-by: Tapani Pälli 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106833
---
  src/compiler/glsl/linker.cpp | 31 ---
  1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index 2f4c8860547..905d4b3cbed 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -2709,7 +2709,8 @@ static bool
  assign_attribute_or_color_locations(void *mem_ctx,
  gl_shader_program *prog,
  struct gl_constants *constants,
-unsigned target_index)
+unsigned target_index,
+bool do_assignment)
  {
 /* Maximum number of generic locations.  This corresponds to either the
  * maximum number of draw buffers or the maximum number of generic
@@ -3072,6 +3073,9 @@ assign_attribute_or_color_locations(void *mem_ctx,
num_attr++;
 }
  
+   if (!do_assignment)

+  return true;
+
 if (target_index == MESA_SHADER_VERTEX) {
unsigned total_attribs_size =
   util_bitcount(used_locations &
SAFE_MASK_FROM_INDEX(max_index)) +
@@ -4780,12 +4784,12 @@ link_varyings_and_uniforms(unsigned first,
unsigned last,
 }
  
 if (!assign_attribute_or_color_locations(mem_ctx, prog, >Const,

-MESA_SHADER_VERTEX)) {
+MESA_SHADER_VERTEX, true)) {
return false;
 }
  
 if (!assign_attribute_or_color_locations(mem_ctx, prog, >Const,

-MESA_SHADER_FRAGMENT)) {
+MESA_SHADER_FRAGMENT, true)) {
return false;
 }
  
@@ -5162,6 +5166,27 @@ link_shaders(struct gl_context *ctx, struct

gl_shader_program *prog)
   lower_tess_level(prog->_LinkedShaders[i]);
}
  
+  /* Section 13.46 (Vertex Attribute Aliasing) of the OpenGL ES 3.2

+   * specification says:
+   *
+   *"In general, the behavior of GLSL ES should not depend on
compiler
+   *optimizations which might be implementation-dependent. Name
matching
+   *rules in most languages, including C++ from which GLSL ES
is derived,
+   *are based on declarations rather than use.
+   *
+   *RESOLUTION: The existence of aliasing is determined by
declarations
+   *present after preprocessing."
+   *
+   * Because of this rule, we do a 'dry-run' of attribute
assignment for
+   * vertex shader inputs here.
+   */
+  if (i == MESA_SHADER_VERTEX) {


Can we please add a GLES check to the if above so we can skip this for desktop 
GL.

Its a little unfortunate we need this patch but splitting 
assign_attribute_or_color_locations() in two would get messy so with the above 
change.

Reviewed-by: Timothy Arceri 


Sure, I'll add a GLES check. Thanks for the review!




+ if (!assign_attribute_or_color_locations(mem_ctx, prog, 

Const,

+  MESA_SHADER_VERTEX,
false)) {
+goto done;
+ }
+  }
+
/* Call opts before lowering const arrays to uniforms so we can
const
 * propagate any elements accessed directly.
 */
--
2.17.1

___
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] glsl/linker: validate attribute aliasing before optimizations

2018-10-26 Thread Timothy Arceri 
On Fri, Oct 12, 2018, at 5:04 AM, Tapani Pälli wrote:
> Patch does a 'dry run' of assign_attribute_or_color_locations before
> optimizations to catch cases where we have aliasing of unused attributes
> which is forbidden by the GLSL ES 3.x specifications.
> 
> We need to run this pass before unused attributes may be removed and with
> attribute binding information from program, therefore we re-use existing
> pass in linker rather than attempt to write another one.
> 
> This fixes WebGL2 test 'gl-bindAttribLocation-aliasing-inactive' and
> Piglit test 'gles-3.0-attribute-aliasing'.
> 
> Signed-off-by: Tapani Pälli 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106833
> ---
>  src/compiler/glsl/linker.cpp | 31 ---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 2f4c8860547..905d4b3cbed 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -2709,7 +2709,8 @@ static bool
>  assign_attribute_or_color_locations(void *mem_ctx,
>  gl_shader_program *prog,
>  struct gl_constants *constants,
> -unsigned target_index)
> +unsigned target_index,
> +bool do_assignment)
>  {
> /* Maximum number of generic locations.  This corresponds to either the
>  * maximum number of draw buffers or the maximum number of generic
> @@ -3072,6 +3073,9 @@ assign_attribute_or_color_locations(void *mem_ctx,
>num_attr++;
> }
>  
> +   if (!do_assignment)
> +  return true;
> +
> if (target_index == MESA_SHADER_VERTEX) {
>unsigned total_attribs_size =
>   util_bitcount(used_locations & 
> SAFE_MASK_FROM_INDEX(max_index)) +
> @@ -4780,12 +4784,12 @@ link_varyings_and_uniforms(unsigned first, 
> unsigned last,
> }
>  
> if (!assign_attribute_or_color_locations(mem_ctx, prog, >Const,
> -MESA_SHADER_VERTEX)) {
> +MESA_SHADER_VERTEX, true)) {
>return false;
> }
>  
> if (!assign_attribute_or_color_locations(mem_ctx, prog, >Const,
> -MESA_SHADER_FRAGMENT)) {
> +MESA_SHADER_FRAGMENT, true)) {
>return false;
> }
>  
> @@ -5162,6 +5166,27 @@ link_shaders(struct gl_context *ctx, struct 
> gl_shader_program *prog)
>   lower_tess_level(prog->_LinkedShaders[i]);
>}
>  
> +  /* Section 13.46 (Vertex Attribute Aliasing) of the OpenGL ES 3.2
> +   * specification says:
> +   *
> +   *"In general, the behavior of GLSL ES should not depend on 
> compiler
> +   *optimizations which might be implementation-dependent. Name 
> matching
> +   *rules in most languages, including C++ from which GLSL ES 
> is derived,
> +   *are based on declarations rather than use.
> +   *
> +   *RESOLUTION: The existence of aliasing is determined by 
> declarations
> +   *present after preprocessing."
> +   *
> +   * Because of this rule, we do a 'dry-run' of attribute 
> assignment for
> +   * vertex shader inputs here.
> +   */
> +  if (i == MESA_SHADER_VERTEX) {

Can we please add a GLES check to the if above so we can skip this for desktop 
GL. 

Its a little unfortunate we need this patch but splitting 
assign_attribute_or_color_locations() in two would get messy so with the above 
change.

Reviewed-by: Timothy Arceri 


> + if (!assign_attribute_or_color_locations(mem_ctx, prog, 
> >Const,
> +  MESA_SHADER_VERTEX, 
> false)) {
> +goto done;
> + }
> +  }
> +
>/* Call opts before lowering const arrays to uniforms so we can 
> const
> * propagate any elements accessed directly.
> */
> -- 
> 2.17.1
> 
> ___
> 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] glsl/linker: validate attribute aliasing before optimizations

2018-10-24 Thread Tapani Pälli 

ping

On 10/12/18 3:04 PM, Tapani Pälli wrote:

Patch does a 'dry run' of assign_attribute_or_color_locations before
optimizations to catch cases where we have aliasing of unused attributes
which is forbidden by the GLSL ES 3.x specifications.

We need to run this pass before unused attributes may be removed and with
attribute binding information from program, therefore we re-use existing
pass in linker rather than attempt to write another one.

This fixes WebGL2 test 'gl-bindAttribLocation-aliasing-inactive' and
Piglit test 'gles-3.0-attribute-aliasing'.

Signed-off-by: Tapani Pälli 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106833
---
  src/compiler/glsl/linker.cpp | 31 ---
  1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index 2f4c8860547..905d4b3cbed 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -2709,7 +2709,8 @@ static bool
  assign_attribute_or_color_locations(void *mem_ctx,
  gl_shader_program *prog,
  struct gl_constants *constants,
-unsigned target_index)
+unsigned target_index,
+bool do_assignment)
  {
 /* Maximum number of generic locations.  This corresponds to either the
  * maximum number of draw buffers or the maximum number of generic
@@ -3072,6 +3073,9 @@ assign_attribute_or_color_locations(void *mem_ctx,
num_attr++;
 }
  
+   if (!do_assignment)

+  return true;
+
 if (target_index == MESA_SHADER_VERTEX) {
unsigned total_attribs_size =
   util_bitcount(used_locations & SAFE_MASK_FROM_INDEX(max_index)) +
@@ -4780,12 +4784,12 @@ link_varyings_and_uniforms(unsigned first, unsigned 
last,
 }
  
 if (!assign_attribute_or_color_locations(mem_ctx, prog, >Const,

-MESA_SHADER_VERTEX)) {
+MESA_SHADER_VERTEX, true)) {
return false;
 }
  
 if (!assign_attribute_or_color_locations(mem_ctx, prog, >Const,

-MESA_SHADER_FRAGMENT)) {
+MESA_SHADER_FRAGMENT, true)) {
return false;
 }
  
@@ -5162,6 +5166,27 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)

   lower_tess_level(prog->_LinkedShaders[i]);
}
  
+  /* Section 13.46 (Vertex Attribute Aliasing) of the OpenGL ES 3.2

+   * specification says:
+   *
+   *"In general, the behavior of GLSL ES should not depend on compiler
+   *optimizations which might be implementation-dependent. Name 
matching
+   *rules in most languages, including C++ from which GLSL ES is 
derived,
+   *are based on declarations rather than use.
+   *
+   *RESOLUTION: The existence of aliasing is determined by declarations
+   *present after preprocessing."
+   *
+   * Because of this rule, we do a 'dry-run' of attribute assignment for
+   * vertex shader inputs here.
+   */
+  if (i == MESA_SHADER_VERTEX) {
+ if (!assign_attribute_or_color_locations(mem_ctx, prog, >Const,
+  MESA_SHADER_VERTEX, false)) {
+goto done;
+ }
+  }
+
/* Call opts before lowering const arrays to uniforms so we can const
 * propagate any elements accessed directly.
 */


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

[Mesa-dev] [PATCH] glsl/linker: validate attribute aliasing before optimizations

2018-10-12 Thread Tapani Pälli 
Patch does a 'dry run' of assign_attribute_or_color_locations before
optimizations to catch cases where we have aliasing of unused attributes
which is forbidden by the GLSL ES 3.x specifications.

We need to run this pass before unused attributes may be removed and with
attribute binding information from program, therefore we re-use existing
pass in linker rather than attempt to write another one.

This fixes WebGL2 test 'gl-bindAttribLocation-aliasing-inactive' and
Piglit test 'gles-3.0-attribute-aliasing'.

Signed-off-by: Tapani Pälli 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106833
---
 src/compiler/glsl/linker.cpp | 31 ---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index 2f4c8860547..905d4b3cbed 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -2709,7 +2709,8 @@ static bool
 assign_attribute_or_color_locations(void *mem_ctx,
 gl_shader_program *prog,
 struct gl_constants *constants,
-unsigned target_index)
+unsigned target_index,
+bool do_assignment)
 {
/* Maximum number of generic locations.  This corresponds to either the
 * maximum number of draw buffers or the maximum number of generic
@@ -3072,6 +3073,9 @@ assign_attribute_or_color_locations(void *mem_ctx,
   num_attr++;
}
 
+   if (!do_assignment)
+  return true;
+
if (target_index == MESA_SHADER_VERTEX) {
   unsigned total_attribs_size =
  util_bitcount(used_locations & SAFE_MASK_FROM_INDEX(max_index)) +
@@ -4780,12 +4784,12 @@ link_varyings_and_uniforms(unsigned first, unsigned 
last,
}
 
if (!assign_attribute_or_color_locations(mem_ctx, prog, >Const,
-MESA_SHADER_VERTEX)) {
+MESA_SHADER_VERTEX, true)) {
   return false;
}
 
if (!assign_attribute_or_color_locations(mem_ctx, prog, >Const,
-MESA_SHADER_FRAGMENT)) {
+MESA_SHADER_FRAGMENT, true)) {
   return false;
}
 
@@ -5162,6 +5166,27 @@ link_shaders(struct gl_context *ctx, struct 
gl_shader_program *prog)
  lower_tess_level(prog->_LinkedShaders[i]);
   }
 
+  /* Section 13.46 (Vertex Attribute Aliasing) of the OpenGL ES 3.2
+   * specification says:
+   *
+   *"In general, the behavior of GLSL ES should not depend on compiler
+   *optimizations which might be implementation-dependent. Name 
matching
+   *rules in most languages, including C++ from which GLSL ES is 
derived,
+   *are based on declarations rather than use.
+   *
+   *RESOLUTION: The existence of aliasing is determined by declarations
+   *present after preprocessing."
+   *
+   * Because of this rule, we do a 'dry-run' of attribute assignment for
+   * vertex shader inputs here.
+   */
+  if (i == MESA_SHADER_VERTEX) {
+ if (!assign_attribute_or_color_locations(mem_ctx, prog, >Const,
+  MESA_SHADER_VERTEX, false)) {
+goto done;
+ }
+  }
+
   /* Call opts before lowering const arrays to uniforms so we can const
* propagate any elements accessed directly.
*/
-- 
2.17.1

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