Re: [Mesa-dev] [PATCH] glsl: remove unecessary flags.q.subroutine_def

2017-03-01 Thread Samuel Pitoiset 
Mark, could you try "[PATCH] glsl: fix subroutine mismatch between 
declarations/definitions" ?


It fixes the piglit regression on my side.

On 03/01/2017 08:36 PM, Mark Janes wrote:

This patch breaks gl45-cts.shader_subroutine.static_subroutine_call.

It also breaks
piglit.spec.arb_shader_subroutine.compiler.direct-call_vert:

Failed to compile vertex shader 
piglit/tests/spec/arb_shader_subroutine/compiler/direct-call.vert: 0:26(2): 
error: subroutine name cannot be a constructor `impl'

Samuel Pitoiset  writes:


This bit is definitely not necessary because subroutine_list
can be used instead. This frees one more bit in the flags.q
struct which is nice because arb_bindless_texture will need
4 bits for the new layout qualifiers.

No piglit regressions found (including compiler tests) with
"-t subroutine".

Signed-off-by: Samuel Pitoiset 
---
 src/compiler/glsl/ast.h  | 1 -
 src/compiler/glsl/ast_to_hir.cpp | 6 +++---
 src/compiler/glsl/ast_type.cpp   | 6 ++
 src/compiler/glsl/glsl_parser.yy | 1 -
 src/compiler/glsl/glsl_parser_extras.cpp | 2 +-
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
index 11a092e41c..d27b940744 100644
--- a/src/compiler/glsl/ast.h
+++ b/src/compiler/glsl/ast.h
@@ -607,7 +607,6 @@ struct ast_type_qualifier {
  /** \name Qualifiers for GL_ARB_shader_subroutine */
 /** \{ */
  unsigned subroutine:1;  /**< Is this marked 'subroutine' */
- unsigned subroutine_def:1; /**< Is this marked 'subroutine' with a 
list of types */
 /** \} */

  /** \name Qualifiers for GL_KHR_blend_equation_advanced */
diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index f033d7df97..7e99faeaed 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -3510,7 +3510,7 @@ apply_layout_qualifier_to_variable(const struct 
ast_type_qualifier *qual,
  }
   }
} else if (qual->flags.q.explicit_index) {
-  if (!qual->flags.q.subroutine_def)
+  if (!qual->subroutine_list)
  _mesa_glsl_error(loc, state,
   "explicit index requires explicit location");
} else if (qual->flags.q.explicit_component) {
@@ -5568,7 +5568,7 @@ ast_function::hir(exec_list *instructions,
 *  "Subroutine declarations cannot be prototyped. It is an error to prepend
 *   subroutine(...) to a function declaration."
 */
-   if (this->return_type->qualifier.flags.q.subroutine_def && !is_definition) {
+   if (this->return_type->qualifier.subroutine_list && !is_definition) {
   YYLTYPE loc = this->get_location();
   _mesa_glsl_error(&loc, state,
"function declaration `%s' cannot have subroutine 
prepended",
@@ -5716,7 +5716,7 @@ ast_function::hir(exec_list *instructions,
sig->replace_parameters(&hir_parameters);
signature = sig;

-   if (this->return_type->qualifier.flags.q.subroutine_def) {
+   if (this->return_type->qualifier.subroutine_list) {
   int idx;

   if (this->return_type->qualifier.flags.q.explicit_index) {
diff --git a/src/compiler/glsl/ast_type.cpp b/src/compiler/glsl/ast_type.cpp
index 96d20c10af..5f868a81f2 100644
--- a/src/compiler/glsl/ast_type.cpp
+++ b/src/compiler/glsl/ast_type.cpp
@@ -44,7 +44,6 @@ 
ast_fully_specified_type::has_qualifiers(_mesa_glsl_parse_state *state) const
ast_type_qualifier subroutine_only;
subroutine_only.flags.i = 0;
subroutine_only.flags.q.subroutine = 1;
-   subroutine_only.flags.q.subroutine_def = 1;
if (state->has_explicit_uniform_location()) {
   subroutine_only.flags.q.explicit_index = 1;
}
@@ -285,8 +284,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
   }
}

-   if (q.flags.q.subroutine_def) {
-  if (this->flags.q.subroutine_def) {
+   if (q.subroutine_list) {
+  if (this->subroutine_list) {
  _mesa_glsl_error(loc, state,
   "conflicting subroutine qualifiers used");
   } else {
@@ -772,7 +771,6 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
 bad.flags.q.point_mode ? " point_mode" : "",
 bad.flags.q.vertices ? " vertices" : "",
 bad.flags.q.subroutine ? " subroutine" : "",
-bad.flags.q.subroutine_def ? " subroutine_def" : "",
 bad.flags.q.blend_support ? " blend_support" : "",
 bad.flags.q.inner_coverage ? " inner_coverage" : "",
 bad.flags.q.post_depth_coverage ? " post_depth_coverage" : 
"");
diff --git a/src/compiler/glsl/glsl_parser.yy b/src/compiler/glsl/glsl_parser.yy
index d703f8..b79fcee550 100644
--- a/src/compiler/glsl/glsl_parser.yy
+++ b/src/compiler/glsl/glsl_parser.yy
@@ -1812,7 +1812,6 @@ subroutine_qualifier:
| SUBROUTINE '(' subroutine_type_list ')'
{
   memset(& $$, 0, sizeof($$));
-  $$.flags.q.subrou

Re: [Mesa-dev] [PATCH] glsl: remove unecessary flags.q.subroutine_def

2017-03-01 Thread Samuel Pitoiset 

Thanks for reporting this. I will investigate.

On 03/01/2017 08:36 PM, Mark Janes wrote:

This patch breaks gl45-cts.shader_subroutine.static_subroutine_call.

It also breaks
piglit.spec.arb_shader_subroutine.compiler.direct-call_vert:

Failed to compile vertex shader 
piglit/tests/spec/arb_shader_subroutine/compiler/direct-call.vert: 0:26(2): 
error: subroutine name cannot be a constructor `impl'

Samuel Pitoiset  writes:


This bit is definitely not necessary because subroutine_list
can be used instead. This frees one more bit in the flags.q
struct which is nice because arb_bindless_texture will need
4 bits for the new layout qualifiers.

No piglit regressions found (including compiler tests) with
"-t subroutine".

Signed-off-by: Samuel Pitoiset 
---
 src/compiler/glsl/ast.h  | 1 -
 src/compiler/glsl/ast_to_hir.cpp | 6 +++---
 src/compiler/glsl/ast_type.cpp   | 6 ++
 src/compiler/glsl/glsl_parser.yy | 1 -
 src/compiler/glsl/glsl_parser_extras.cpp | 2 +-
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
index 11a092e41c..d27b940744 100644
--- a/src/compiler/glsl/ast.h
+++ b/src/compiler/glsl/ast.h
@@ -607,7 +607,6 @@ struct ast_type_qualifier {
  /** \name Qualifiers for GL_ARB_shader_subroutine */
 /** \{ */
  unsigned subroutine:1;  /**< Is this marked 'subroutine' */
- unsigned subroutine_def:1; /**< Is this marked 'subroutine' with a 
list of types */
 /** \} */

  /** \name Qualifiers for GL_KHR_blend_equation_advanced */
diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index f033d7df97..7e99faeaed 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -3510,7 +3510,7 @@ apply_layout_qualifier_to_variable(const struct 
ast_type_qualifier *qual,
  }
   }
} else if (qual->flags.q.explicit_index) {
-  if (!qual->flags.q.subroutine_def)
+  if (!qual->subroutine_list)
  _mesa_glsl_error(loc, state,
   "explicit index requires explicit location");
} else if (qual->flags.q.explicit_component) {
@@ -5568,7 +5568,7 @@ ast_function::hir(exec_list *instructions,
 *  "Subroutine declarations cannot be prototyped. It is an error to prepend
 *   subroutine(...) to a function declaration."
 */
-   if (this->return_type->qualifier.flags.q.subroutine_def && !is_definition) {
+   if (this->return_type->qualifier.subroutine_list && !is_definition) {
   YYLTYPE loc = this->get_location();
   _mesa_glsl_error(&loc, state,
"function declaration `%s' cannot have subroutine 
prepended",
@@ -5716,7 +5716,7 @@ ast_function::hir(exec_list *instructions,
sig->replace_parameters(&hir_parameters);
signature = sig;

-   if (this->return_type->qualifier.flags.q.subroutine_def) {
+   if (this->return_type->qualifier.subroutine_list) {
   int idx;

   if (this->return_type->qualifier.flags.q.explicit_index) {
diff --git a/src/compiler/glsl/ast_type.cpp b/src/compiler/glsl/ast_type.cpp
index 96d20c10af..5f868a81f2 100644
--- a/src/compiler/glsl/ast_type.cpp
+++ b/src/compiler/glsl/ast_type.cpp
@@ -44,7 +44,6 @@ 
ast_fully_specified_type::has_qualifiers(_mesa_glsl_parse_state *state) const
ast_type_qualifier subroutine_only;
subroutine_only.flags.i = 0;
subroutine_only.flags.q.subroutine = 1;
-   subroutine_only.flags.q.subroutine_def = 1;
if (state->has_explicit_uniform_location()) {
   subroutine_only.flags.q.explicit_index = 1;
}
@@ -285,8 +284,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
   }
}

-   if (q.flags.q.subroutine_def) {
-  if (this->flags.q.subroutine_def) {
+   if (q.subroutine_list) {
+  if (this->subroutine_list) {
  _mesa_glsl_error(loc, state,
   "conflicting subroutine qualifiers used");
   } else {
@@ -772,7 +771,6 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
 bad.flags.q.point_mode ? " point_mode" : "",
 bad.flags.q.vertices ? " vertices" : "",
 bad.flags.q.subroutine ? " subroutine" : "",
-bad.flags.q.subroutine_def ? " subroutine_def" : "",
 bad.flags.q.blend_support ? " blend_support" : "",
 bad.flags.q.inner_coverage ? " inner_coverage" : "",
 bad.flags.q.post_depth_coverage ? " post_depth_coverage" : 
"");
diff --git a/src/compiler/glsl/glsl_parser.yy b/src/compiler/glsl/glsl_parser.yy
index d703f8..b79fcee550 100644
--- a/src/compiler/glsl/glsl_parser.yy
+++ b/src/compiler/glsl/glsl_parser.yy
@@ -1812,7 +1812,6 @@ subroutine_qualifier:
| SUBROUTINE '(' subroutine_type_list ')'
{
   memset(& $$, 0, sizeof($$));
-  $$.flags.q.subroutine_def = 1;
   $$.subroutine_list = $3;
}
;
diff --git a/src/compiler/glsl/glsl

Re: [Mesa-dev] [PATCH] glsl: remove unecessary flags.q.subroutine_def

2017-03-01 Thread Mark Janes 
This patch breaks gl45-cts.shader_subroutine.static_subroutine_call.

It also breaks
piglit.spec.arb_shader_subroutine.compiler.direct-call_vert:

Failed to compile vertex shader 
piglit/tests/spec/arb_shader_subroutine/compiler/direct-call.vert: 0:26(2): 
error: subroutine name cannot be a constructor `impl'

Samuel Pitoiset  writes:

> This bit is definitely not necessary because subroutine_list
> can be used instead. This frees one more bit in the flags.q
> struct which is nice because arb_bindless_texture will need
> 4 bits for the new layout qualifiers.
>
> No piglit regressions found (including compiler tests) with
> "-t subroutine".
>
> Signed-off-by: Samuel Pitoiset 
> ---
>  src/compiler/glsl/ast.h  | 1 -
>  src/compiler/glsl/ast_to_hir.cpp | 6 +++---
>  src/compiler/glsl/ast_type.cpp   | 6 ++
>  src/compiler/glsl/glsl_parser.yy | 1 -
>  src/compiler/glsl/glsl_parser_extras.cpp | 2 +-
>  5 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> index 11a092e41c..d27b940744 100644
> --- a/src/compiler/glsl/ast.h
> +++ b/src/compiler/glsl/ast.h
> @@ -607,7 +607,6 @@ struct ast_type_qualifier {
>   /** \name Qualifiers for GL_ARB_shader_subroutine */
>/** \{ */
>   unsigned subroutine:1;  /**< Is this marked 'subroutine' */
> - unsigned subroutine_def:1; /**< Is this marked 'subroutine' with a 
> list of types */
>/** \} */
>  
>   /** \name Qualifiers for GL_KHR_blend_equation_advanced */
> diff --git a/src/compiler/glsl/ast_to_hir.cpp 
> b/src/compiler/glsl/ast_to_hir.cpp
> index f033d7df97..7e99faeaed 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -3510,7 +3510,7 @@ apply_layout_qualifier_to_variable(const struct 
> ast_type_qualifier *qual,
>   }
>}
> } else if (qual->flags.q.explicit_index) {
> -  if (!qual->flags.q.subroutine_def)
> +  if (!qual->subroutine_list)
>   _mesa_glsl_error(loc, state,
>"explicit index requires explicit location");
> } else if (qual->flags.q.explicit_component) {
> @@ -5568,7 +5568,7 @@ ast_function::hir(exec_list *instructions,
>  *  "Subroutine declarations cannot be prototyped. It is an error to 
> prepend
>  *   subroutine(...) to a function declaration."
>  */
> -   if (this->return_type->qualifier.flags.q.subroutine_def && 
> !is_definition) {
> +   if (this->return_type->qualifier.subroutine_list && !is_definition) {
>YYLTYPE loc = this->get_location();
>_mesa_glsl_error(&loc, state,
> "function declaration `%s' cannot have subroutine 
> prepended",
> @@ -5716,7 +5716,7 @@ ast_function::hir(exec_list *instructions,
> sig->replace_parameters(&hir_parameters);
> signature = sig;
>  
> -   if (this->return_type->qualifier.flags.q.subroutine_def) {
> +   if (this->return_type->qualifier.subroutine_list) {
>int idx;
>  
>if (this->return_type->qualifier.flags.q.explicit_index) {
> diff --git a/src/compiler/glsl/ast_type.cpp b/src/compiler/glsl/ast_type.cpp
> index 96d20c10af..5f868a81f2 100644
> --- a/src/compiler/glsl/ast_type.cpp
> +++ b/src/compiler/glsl/ast_type.cpp
> @@ -44,7 +44,6 @@ 
> ast_fully_specified_type::has_qualifiers(_mesa_glsl_parse_state *state) const
> ast_type_qualifier subroutine_only;
> subroutine_only.flags.i = 0;
> subroutine_only.flags.q.subroutine = 1;
> -   subroutine_only.flags.q.subroutine_def = 1;
> if (state->has_explicit_uniform_location()) {
>subroutine_only.flags.q.explicit_index = 1;
> }
> @@ -285,8 +284,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>}
> }
>  
> -   if (q.flags.q.subroutine_def) {
> -  if (this->flags.q.subroutine_def) {
> +   if (q.subroutine_list) {
> +  if (this->subroutine_list) {
>   _mesa_glsl_error(loc, state,
>"conflicting subroutine qualifiers used");
>} else {
> @@ -772,7 +771,6 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
>  bad.flags.q.point_mode ? " point_mode" : "",
>  bad.flags.q.vertices ? " vertices" : "",
>  bad.flags.q.subroutine ? " subroutine" : "",
> -bad.flags.q.subroutine_def ? " subroutine_def" : "",
>  bad.flags.q.blend_support ? " blend_support" : "",
>  bad.flags.q.inner_coverage ? " inner_coverage" : "",
>  bad.flags.q.post_depth_coverage ? " post_depth_coverage" 
> : "");
> diff --git a/src/compiler/glsl/glsl_parser.yy 
> b/src/compiler/glsl/glsl_parser.yy
> index d703f8..b79fcee550 100644
> --- a/src/compiler/glsl/glsl_parser.yy
> +++ b/src/compiler/glsl/glsl_parser.yy
> @@ -1812,7 +1812,6 @@ subroutine_qualifier:
> | SUBROUTINE '(' subroutine_type_list ')'
> {
>memset(& $$, 0, sizeof

Re: [Mesa-dev] [PATCH] glsl: remove unecessary flags.q.subroutine_def

2017-02-28 Thread Samuel Pitoiset 



On 03/01/2017 12:04 AM, Timothy Arceri wrote:

On 01/03/17 03:57, Samuel Pitoiset wrote:



On 02/26/2017 10:19 PM, Timothy Arceri wrote:



On 25/02/17 22:15, Samuel Pitoiset wrote:

This bit is definitely not necessary because subroutine_list
can be used instead. This frees one more bit in the flags.q
struct which is nice because arb_bindless_texture will need
4 bits for the new layout qualifiers.

No piglit regressions found (including compiler tests) with
"-t subroutine".

Signed-off-by: Samuel Pitoiset 
---
 src/compiler/glsl/ast.h  | 1 -
 src/compiler/glsl/ast_to_hir.cpp | 6 +++---
 src/compiler/glsl/ast_type.cpp   | 6 ++
 src/compiler/glsl/glsl_parser.yy | 1 -
 src/compiler/glsl/glsl_parser_extras.cpp | 2 +-
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
index 11a092e41c..d27b940744 100644
--- a/src/compiler/glsl/ast.h
+++ b/src/compiler/glsl/ast.h
@@ -607,7 +607,6 @@ struct ast_type_qualifier {
  /** \name Qualifiers for GL_ARB_shader_subroutine */
  /** \{ */
  unsigned subroutine:1;  /**< Is this marked 'subroutine' */
- unsigned subroutine_def:1; /**< Is this marked 'subroutine'
with a list of types */
  /** \} */

  /** \name Qualifiers for GL_KHR_blend_equation_advanced */
diff --git a/src/compiler/glsl/ast_to_hir.cpp
b/src/compiler/glsl/ast_to_hir.cpp
index f033d7df97..7e99faeaed 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -3510,7 +3510,7 @@ apply_layout_qualifier_to_variable(const struct
ast_type_qualifier *qual,
  }
   }
} else if (qual->flags.q.explicit_index) {
-  if (!qual->flags.q.subroutine_def)
+  if (!qual->subroutine_list)
  _mesa_glsl_error(loc, state,
   "explicit index requires explicit
location");
} else if (qual->flags.q.explicit_component) {
@@ -5568,7 +5568,7 @@ ast_function::hir(exec_list *instructions,
 *  "Subroutine declarations cannot be prototyped. It is an error
to prepend
 *   subroutine(...) to a function declaration."
 */
-   if (this->return_type->qualifier.flags.q.subroutine_def &&
!is_definition) {
+   if (this->return_type->qualifier.subroutine_list &&
!is_definition) {
   YYLTYPE loc = this->get_location();
   _mesa_glsl_error(&loc, state,
"function declaration `%s' cannot have
subroutine prepended",
@@ -5716,7 +5716,7 @@ ast_function::hir(exec_list *instructions,
sig->replace_parameters(&hir_parameters);
signature = sig;

-   if (this->return_type->qualifier.flags.q.subroutine_def) {
+   if (this->return_type->qualifier.subroutine_list) {
   int idx;

   if (this->return_type->qualifier.flags.q.explicit_index) {
diff --git a/src/compiler/glsl/ast_type.cpp
b/src/compiler/glsl/ast_type.cpp
index 96d20c10af..5f868a81f2 100644
--- a/src/compiler/glsl/ast_type.cpp
+++ b/src/compiler/glsl/ast_type.cpp
@@ -44,7 +44,6 @@
ast_fully_specified_type::has_qualifiers(_mesa_glsl_parse_state
*state) const
ast_type_qualifier subroutine_only;
subroutine_only.flags.i = 0;
subroutine_only.flags.q.subroutine = 1;
-   subroutine_only.flags.q.subroutine_def = 1;
if (state->has_explicit_uniform_location()) {
   subroutine_only.flags.q.explicit_index = 1;
}
@@ -285,8 +284,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
   }
}

-   if (q.flags.q.subroutine_def) {
-  if (this->flags.q.subroutine_def) {
+   if (q.subroutine_list) {
+  if (this->subroutine_list) {
  _mesa_glsl_error(loc, state,
   "conflicting subroutine qualifiers used");
   } else {
@@ -772,7 +771,6 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
 bad.flags.q.point_mode ? " point_mode" : "",
 bad.flags.q.vertices ? " vertices" : "",
 bad.flags.q.subroutine ? " subroutine" : "",
-bad.flags.q.subroutine_def ? " subroutine_def" :
"",
 bad.flags.q.blend_support ? " blend_support" : "",
 bad.flags.q.inner_coverage ? " inner_coverage" :
"",
 bad.flags.q.post_depth_coverage ? "
post_depth_coverage" : "");
diff --git a/src/compiler/glsl/glsl_parser.yy
b/src/compiler/glsl/glsl_parser.yy
index d703f8..b79fcee550 100644
--- a/src/compiler/glsl/glsl_parser.yy
+++ b/src/compiler/glsl/glsl_parser.yy
@@ -1812,7 +1812,6 @@ subroutine_qualifier:
| SUBROUTINE '(' subroutine_type_list ')'
{
   memset(& $$, 0, sizeof($$));
-  $$.flags.q.subroutine_def = 1;


You need to change this to:

   $$.flags.q.subroutine = 1;

Otherwise we won't detect if the qualifier was added when it should have
been etc.


Are you sure it's needed to set it up? I thought it was enough to only
set $$.subroutine_list.


Sorry I should have been more clear. I mean the function that make use
of checking the qua

Re: [Mesa-dev] [PATCH] glsl: remove unecessary flags.q.subroutine_def

2017-02-28 Thread Timothy Arceri 

On 01/03/17 03:57, Samuel Pitoiset wrote:



On 02/26/2017 10:19 PM, Timothy Arceri wrote:



On 25/02/17 22:15, Samuel Pitoiset wrote:

This bit is definitely not necessary because subroutine_list
can be used instead. This frees one more bit in the flags.q
struct which is nice because arb_bindless_texture will need
4 bits for the new layout qualifiers.

No piglit regressions found (including compiler tests) with
"-t subroutine".

Signed-off-by: Samuel Pitoiset 
---
 src/compiler/glsl/ast.h  | 1 -
 src/compiler/glsl/ast_to_hir.cpp | 6 +++---
 src/compiler/glsl/ast_type.cpp   | 6 ++
 src/compiler/glsl/glsl_parser.yy | 1 -
 src/compiler/glsl/glsl_parser_extras.cpp | 2 +-
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
index 11a092e41c..d27b940744 100644
--- a/src/compiler/glsl/ast.h
+++ b/src/compiler/glsl/ast.h
@@ -607,7 +607,6 @@ struct ast_type_qualifier {
  /** \name Qualifiers for GL_ARB_shader_subroutine */
  /** \{ */
  unsigned subroutine:1;  /**< Is this marked 'subroutine' */
- unsigned subroutine_def:1; /**< Is this marked 'subroutine'
with a list of types */
  /** \} */

  /** \name Qualifiers for GL_KHR_blend_equation_advanced */
diff --git a/src/compiler/glsl/ast_to_hir.cpp
b/src/compiler/glsl/ast_to_hir.cpp
index f033d7df97..7e99faeaed 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -3510,7 +3510,7 @@ apply_layout_qualifier_to_variable(const struct
ast_type_qualifier *qual,
  }
   }
} else if (qual->flags.q.explicit_index) {
-  if (!qual->flags.q.subroutine_def)
+  if (!qual->subroutine_list)
  _mesa_glsl_error(loc, state,
   "explicit index requires explicit location");
} else if (qual->flags.q.explicit_component) {
@@ -5568,7 +5568,7 @@ ast_function::hir(exec_list *instructions,
 *  "Subroutine declarations cannot be prototyped. It is an error
to prepend
 *   subroutine(...) to a function declaration."
 */
-   if (this->return_type->qualifier.flags.q.subroutine_def &&
!is_definition) {
+   if (this->return_type->qualifier.subroutine_list &&
!is_definition) {
   YYLTYPE loc = this->get_location();
   _mesa_glsl_error(&loc, state,
"function declaration `%s' cannot have
subroutine prepended",
@@ -5716,7 +5716,7 @@ ast_function::hir(exec_list *instructions,
sig->replace_parameters(&hir_parameters);
signature = sig;

-   if (this->return_type->qualifier.flags.q.subroutine_def) {
+   if (this->return_type->qualifier.subroutine_list) {
   int idx;

   if (this->return_type->qualifier.flags.q.explicit_index) {
diff --git a/src/compiler/glsl/ast_type.cpp
b/src/compiler/glsl/ast_type.cpp
index 96d20c10af..5f868a81f2 100644
--- a/src/compiler/glsl/ast_type.cpp
+++ b/src/compiler/glsl/ast_type.cpp
@@ -44,7 +44,6 @@
ast_fully_specified_type::has_qualifiers(_mesa_glsl_parse_state
*state) const
ast_type_qualifier subroutine_only;
subroutine_only.flags.i = 0;
subroutine_only.flags.q.subroutine = 1;
-   subroutine_only.flags.q.subroutine_def = 1;
if (state->has_explicit_uniform_location()) {
   subroutine_only.flags.q.explicit_index = 1;
}
@@ -285,8 +284,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
   }
}

-   if (q.flags.q.subroutine_def) {
-  if (this->flags.q.subroutine_def) {
+   if (q.subroutine_list) {
+  if (this->subroutine_list) {
  _mesa_glsl_error(loc, state,
   "conflicting subroutine qualifiers used");
   } else {
@@ -772,7 +771,6 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
 bad.flags.q.point_mode ? " point_mode" : "",
 bad.flags.q.vertices ? " vertices" : "",
 bad.flags.q.subroutine ? " subroutine" : "",
-bad.flags.q.subroutine_def ? " subroutine_def" :
"",
 bad.flags.q.blend_support ? " blend_support" : "",
 bad.flags.q.inner_coverage ? " inner_coverage" :
"",
 bad.flags.q.post_depth_coverage ? "
post_depth_coverage" : "");
diff --git a/src/compiler/glsl/glsl_parser.yy
b/src/compiler/glsl/glsl_parser.yy
index d703f8..b79fcee550 100644
--- a/src/compiler/glsl/glsl_parser.yy
+++ b/src/compiler/glsl/glsl_parser.yy
@@ -1812,7 +1812,6 @@ subroutine_qualifier:
| SUBROUTINE '(' subroutine_type_list ')'
{
   memset(& $$, 0, sizeof($$));
-  $$.flags.q.subroutine_def = 1;


You need to change this to:

   $$.flags.q.subroutine = 1;

Otherwise we won't detect if the qualifier was added when it should have
been etc.


Are you sure it's needed to set it up? I thought it was enough to only
set $$.subroutine_list.


Sorry I should have been more clear. I mean the function that make use 
of checking the qualifier flags for validation won't work if you do

Re: [Mesa-dev] [PATCH] glsl: remove unecessary flags.q.subroutine_def

2017-02-28 Thread Samuel Pitoiset 



On 02/26/2017 10:19 PM, Timothy Arceri wrote:



On 25/02/17 22:15, Samuel Pitoiset wrote:

This bit is definitely not necessary because subroutine_list
can be used instead. This frees one more bit in the flags.q
struct which is nice because arb_bindless_texture will need
4 bits for the new layout qualifiers.

No piglit regressions found (including compiler tests) with
"-t subroutine".

Signed-off-by: Samuel Pitoiset 
---
 src/compiler/glsl/ast.h  | 1 -
 src/compiler/glsl/ast_to_hir.cpp | 6 +++---
 src/compiler/glsl/ast_type.cpp   | 6 ++
 src/compiler/glsl/glsl_parser.yy | 1 -
 src/compiler/glsl/glsl_parser_extras.cpp | 2 +-
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
index 11a092e41c..d27b940744 100644
--- a/src/compiler/glsl/ast.h
+++ b/src/compiler/glsl/ast.h
@@ -607,7 +607,6 @@ struct ast_type_qualifier {
  /** \name Qualifiers for GL_ARB_shader_subroutine */
  /** \{ */
  unsigned subroutine:1;  /**< Is this marked 'subroutine' */
- unsigned subroutine_def:1; /**< Is this marked 'subroutine'
with a list of types */
  /** \} */

  /** \name Qualifiers for GL_KHR_blend_equation_advanced */
diff --git a/src/compiler/glsl/ast_to_hir.cpp
b/src/compiler/glsl/ast_to_hir.cpp
index f033d7df97..7e99faeaed 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -3510,7 +3510,7 @@ apply_layout_qualifier_to_variable(const struct
ast_type_qualifier *qual,
  }
   }
} else if (qual->flags.q.explicit_index) {
-  if (!qual->flags.q.subroutine_def)
+  if (!qual->subroutine_list)
  _mesa_glsl_error(loc, state,
   "explicit index requires explicit location");
} else if (qual->flags.q.explicit_component) {
@@ -5568,7 +5568,7 @@ ast_function::hir(exec_list *instructions,
 *  "Subroutine declarations cannot be prototyped. It is an error
to prepend
 *   subroutine(...) to a function declaration."
 */
-   if (this->return_type->qualifier.flags.q.subroutine_def &&
!is_definition) {
+   if (this->return_type->qualifier.subroutine_list && !is_definition) {
   YYLTYPE loc = this->get_location();
   _mesa_glsl_error(&loc, state,
"function declaration `%s' cannot have
subroutine prepended",
@@ -5716,7 +5716,7 @@ ast_function::hir(exec_list *instructions,
sig->replace_parameters(&hir_parameters);
signature = sig;

-   if (this->return_type->qualifier.flags.q.subroutine_def) {
+   if (this->return_type->qualifier.subroutine_list) {
   int idx;

   if (this->return_type->qualifier.flags.q.explicit_index) {
diff --git a/src/compiler/glsl/ast_type.cpp
b/src/compiler/glsl/ast_type.cpp
index 96d20c10af..5f868a81f2 100644
--- a/src/compiler/glsl/ast_type.cpp
+++ b/src/compiler/glsl/ast_type.cpp
@@ -44,7 +44,6 @@
ast_fully_specified_type::has_qualifiers(_mesa_glsl_parse_state
*state) const
ast_type_qualifier subroutine_only;
subroutine_only.flags.i = 0;
subroutine_only.flags.q.subroutine = 1;
-   subroutine_only.flags.q.subroutine_def = 1;
if (state->has_explicit_uniform_location()) {
   subroutine_only.flags.q.explicit_index = 1;
}
@@ -285,8 +284,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
   }
}

-   if (q.flags.q.subroutine_def) {
-  if (this->flags.q.subroutine_def) {
+   if (q.subroutine_list) {
+  if (this->subroutine_list) {
  _mesa_glsl_error(loc, state,
   "conflicting subroutine qualifiers used");
   } else {
@@ -772,7 +771,6 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
 bad.flags.q.point_mode ? " point_mode" : "",
 bad.flags.q.vertices ? " vertices" : "",
 bad.flags.q.subroutine ? " subroutine" : "",
-bad.flags.q.subroutine_def ? " subroutine_def" : "",
 bad.flags.q.blend_support ? " blend_support" : "",
 bad.flags.q.inner_coverage ? " inner_coverage" : "",
 bad.flags.q.post_depth_coverage ? "
post_depth_coverage" : "");
diff --git a/src/compiler/glsl/glsl_parser.yy
b/src/compiler/glsl/glsl_parser.yy
index d703f8..b79fcee550 100644
--- a/src/compiler/glsl/glsl_parser.yy
+++ b/src/compiler/glsl/glsl_parser.yy
@@ -1812,7 +1812,6 @@ subroutine_qualifier:
| SUBROUTINE '(' subroutine_type_list ')'
{
   memset(& $$, 0, sizeof($$));
-  $$.flags.q.subroutine_def = 1;


You need to change this to:

   $$.flags.q.subroutine = 1;

Otherwise we won't detect if the qualifier was added when it should have
been etc.


Are you sure it's needed to set it up? I thought it was enough to only 
set $$.subroutine_list.






   $$.subroutine_list = $3;
}
;
diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
b/src/compiler/glsl/glsl_parser_extras.cpp
index 375a99a49d..e88dd071b3 100644

Re: [Mesa-dev] [PATCH] glsl: remove unecessary flags.q.subroutine_def

2017-02-26 Thread Timothy Arceri 



On 25/02/17 22:15, Samuel Pitoiset wrote:

This bit is definitely not necessary because subroutine_list
can be used instead. This frees one more bit in the flags.q
struct which is nice because arb_bindless_texture will need
4 bits for the new layout qualifiers.

No piglit regressions found (including compiler tests) with
"-t subroutine".

Signed-off-by: Samuel Pitoiset 
---
 src/compiler/glsl/ast.h  | 1 -
 src/compiler/glsl/ast_to_hir.cpp | 6 +++---
 src/compiler/glsl/ast_type.cpp   | 6 ++
 src/compiler/glsl/glsl_parser.yy | 1 -
 src/compiler/glsl/glsl_parser_extras.cpp | 2 +-
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
index 11a092e41c..d27b940744 100644
--- a/src/compiler/glsl/ast.h
+++ b/src/compiler/glsl/ast.h
@@ -607,7 +607,6 @@ struct ast_type_qualifier {
  /** \name Qualifiers for GL_ARB_shader_subroutine */
 /** \{ */
  unsigned subroutine:1;  /**< Is this marked 'subroutine' */
- unsigned subroutine_def:1; /**< Is this marked 'subroutine' with a 
list of types */
 /** \} */

  /** \name Qualifiers for GL_KHR_blend_equation_advanced */
diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index f033d7df97..7e99faeaed 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -3510,7 +3510,7 @@ apply_layout_qualifier_to_variable(const struct 
ast_type_qualifier *qual,
  }
   }
} else if (qual->flags.q.explicit_index) {
-  if (!qual->flags.q.subroutine_def)
+  if (!qual->subroutine_list)
  _mesa_glsl_error(loc, state,
   "explicit index requires explicit location");
} else if (qual->flags.q.explicit_component) {
@@ -5568,7 +5568,7 @@ ast_function::hir(exec_list *instructions,
 *  "Subroutine declarations cannot be prototyped. It is an error to prepend
 *   subroutine(...) to a function declaration."
 */
-   if (this->return_type->qualifier.flags.q.subroutine_def && !is_definition) {
+   if (this->return_type->qualifier.subroutine_list && !is_definition) {
   YYLTYPE loc = this->get_location();
   _mesa_glsl_error(&loc, state,
"function declaration `%s' cannot have subroutine 
prepended",
@@ -5716,7 +5716,7 @@ ast_function::hir(exec_list *instructions,
sig->replace_parameters(&hir_parameters);
signature = sig;

-   if (this->return_type->qualifier.flags.q.subroutine_def) {
+   if (this->return_type->qualifier.subroutine_list) {
   int idx;

   if (this->return_type->qualifier.flags.q.explicit_index) {
diff --git a/src/compiler/glsl/ast_type.cpp b/src/compiler/glsl/ast_type.cpp
index 96d20c10af..5f868a81f2 100644
--- a/src/compiler/glsl/ast_type.cpp
+++ b/src/compiler/glsl/ast_type.cpp
@@ -44,7 +44,6 @@ 
ast_fully_specified_type::has_qualifiers(_mesa_glsl_parse_state *state) const
ast_type_qualifier subroutine_only;
subroutine_only.flags.i = 0;
subroutine_only.flags.q.subroutine = 1;
-   subroutine_only.flags.q.subroutine_def = 1;
if (state->has_explicit_uniform_location()) {
   subroutine_only.flags.q.explicit_index = 1;
}
@@ -285,8 +284,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
   }
}

-   if (q.flags.q.subroutine_def) {
-  if (this->flags.q.subroutine_def) {
+   if (q.subroutine_list) {
+  if (this->subroutine_list) {
  _mesa_glsl_error(loc, state,
   "conflicting subroutine qualifiers used");
   } else {
@@ -772,7 +771,6 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
 bad.flags.q.point_mode ? " point_mode" : "",
 bad.flags.q.vertices ? " vertices" : "",
 bad.flags.q.subroutine ? " subroutine" : "",
-bad.flags.q.subroutine_def ? " subroutine_def" : "",
 bad.flags.q.blend_support ? " blend_support" : "",
 bad.flags.q.inner_coverage ? " inner_coverage" : "",
 bad.flags.q.post_depth_coverage ? " post_depth_coverage" : 
"");
diff --git a/src/compiler/glsl/glsl_parser.yy b/src/compiler/glsl/glsl_parser.yy
index d703f8..b79fcee550 100644
--- a/src/compiler/glsl/glsl_parser.yy
+++ b/src/compiler/glsl/glsl_parser.yy
@@ -1812,7 +1812,6 @@ subroutine_qualifier:
| SUBROUTINE '(' subroutine_type_list ')'
{
   memset(& $$, 0, sizeof($$));
-  $$.flags.q.subroutine_def = 1;


You need to change this to:

   $$.flags.q.subroutine = 1;

Otherwise we won't detect if the qualifier was added when it should have 
been etc.




   $$.subroutine_list = $3;
}
;
diff --git a/src/compiler/glsl/glsl_parser_extras.cpp 
b/src/compiler/glsl/glsl_parser_extras.cpp
index 375a99a49d..e88dd071b3 100644
--- a/src/compiler/glsl/glsl_parser_extras.cpp
+++ b/src/compiler/glsl/glsl_parser_extras.cpp
@@ -1075,7 +1075,7 @@ _mesa_ast_type_quali

[Mesa-dev] [PATCH] glsl: remove unecessary flags.q.subroutine_def

2017-02-25 Thread Samuel Pitoiset 
This bit is definitely not necessary because subroutine_list
can be used instead. This frees one more bit in the flags.q
struct which is nice because arb_bindless_texture will need
4 bits for the new layout qualifiers.

No piglit regressions found (including compiler tests) with
"-t subroutine".

Signed-off-by: Samuel Pitoiset 
---
 src/compiler/glsl/ast.h  | 1 -
 src/compiler/glsl/ast_to_hir.cpp | 6 +++---
 src/compiler/glsl/ast_type.cpp   | 6 ++
 src/compiler/glsl/glsl_parser.yy | 1 -
 src/compiler/glsl/glsl_parser_extras.cpp | 2 +-
 5 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
index 11a092e41c..d27b940744 100644
--- a/src/compiler/glsl/ast.h
+++ b/src/compiler/glsl/ast.h
@@ -607,7 +607,6 @@ struct ast_type_qualifier {
  /** \name Qualifiers for GL_ARB_shader_subroutine */
 /** \{ */
  unsigned subroutine:1;  /**< Is this marked 'subroutine' */
- unsigned subroutine_def:1; /**< Is this marked 'subroutine' with a 
list of types */
 /** \} */
 
  /** \name Qualifiers for GL_KHR_blend_equation_advanced */
diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
index f033d7df97..7e99faeaed 100644
--- a/src/compiler/glsl/ast_to_hir.cpp
+++ b/src/compiler/glsl/ast_to_hir.cpp
@@ -3510,7 +3510,7 @@ apply_layout_qualifier_to_variable(const struct 
ast_type_qualifier *qual,
  }
   }
} else if (qual->flags.q.explicit_index) {
-  if (!qual->flags.q.subroutine_def)
+  if (!qual->subroutine_list)
  _mesa_glsl_error(loc, state,
   "explicit index requires explicit location");
} else if (qual->flags.q.explicit_component) {
@@ -5568,7 +5568,7 @@ ast_function::hir(exec_list *instructions,
 *  "Subroutine declarations cannot be prototyped. It is an error to prepend
 *   subroutine(...) to a function declaration."
 */
-   if (this->return_type->qualifier.flags.q.subroutine_def && !is_definition) {
+   if (this->return_type->qualifier.subroutine_list && !is_definition) {
   YYLTYPE loc = this->get_location();
   _mesa_glsl_error(&loc, state,
"function declaration `%s' cannot have subroutine 
prepended",
@@ -5716,7 +5716,7 @@ ast_function::hir(exec_list *instructions,
sig->replace_parameters(&hir_parameters);
signature = sig;
 
-   if (this->return_type->qualifier.flags.q.subroutine_def) {
+   if (this->return_type->qualifier.subroutine_list) {
   int idx;
 
   if (this->return_type->qualifier.flags.q.explicit_index) {
diff --git a/src/compiler/glsl/ast_type.cpp b/src/compiler/glsl/ast_type.cpp
index 96d20c10af..5f868a81f2 100644
--- a/src/compiler/glsl/ast_type.cpp
+++ b/src/compiler/glsl/ast_type.cpp
@@ -44,7 +44,6 @@ 
ast_fully_specified_type::has_qualifiers(_mesa_glsl_parse_state *state) const
ast_type_qualifier subroutine_only;
subroutine_only.flags.i = 0;
subroutine_only.flags.q.subroutine = 1;
-   subroutine_only.flags.q.subroutine_def = 1;
if (state->has_explicit_uniform_location()) {
   subroutine_only.flags.q.explicit_index = 1;
}
@@ -285,8 +284,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
   }
}
 
-   if (q.flags.q.subroutine_def) {
-  if (this->flags.q.subroutine_def) {
+   if (q.subroutine_list) {
+  if (this->subroutine_list) {
  _mesa_glsl_error(loc, state,
   "conflicting subroutine qualifiers used");
   } else {
@@ -772,7 +771,6 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
 bad.flags.q.point_mode ? " point_mode" : "",
 bad.flags.q.vertices ? " vertices" : "",
 bad.flags.q.subroutine ? " subroutine" : "",
-bad.flags.q.subroutine_def ? " subroutine_def" : "",
 bad.flags.q.blend_support ? " blend_support" : "",
 bad.flags.q.inner_coverage ? " inner_coverage" : "",
 bad.flags.q.post_depth_coverage ? " post_depth_coverage" : 
"");
diff --git a/src/compiler/glsl/glsl_parser.yy b/src/compiler/glsl/glsl_parser.yy
index d703f8..b79fcee550 100644
--- a/src/compiler/glsl/glsl_parser.yy
+++ b/src/compiler/glsl/glsl_parser.yy
@@ -1812,7 +1812,6 @@ subroutine_qualifier:
| SUBROUTINE '(' subroutine_type_list ')'
{
   memset(& $$, 0, sizeof($$));
-  $$.flags.q.subroutine_def = 1;
   $$.subroutine_list = $3;
}
;
diff --git a/src/compiler/glsl/glsl_parser_extras.cpp 
b/src/compiler/glsl/glsl_parser_extras.cpp
index 375a99a49d..e88dd071b3 100644
--- a/src/compiler/glsl/glsl_parser_extras.cpp
+++ b/src/compiler/glsl/glsl_parser_extras.cpp
@@ -1075,7 +1075,7 @@ _mesa_ast_type_qualifier_print(const struct 
ast_type_qualifier *q)
if (q->flags.q.subroutine)
   printf("subroutine ");
 
-   if (q->flags.q.subroutine_def) {
+   if (q->subroutine_list) {
   printf(