Re: [Mesa-dev] [PATCH v3 13/14] glsl: add subroutine index qualifier support

2015-11-20 Thread Tapani Pälli 

Reviewed-by: Tapani Pälli 

On 11/14/2015 03:42 PM, Timothy Arceri wrote:

From: Timothy Arceri 

ARB_explicit_uniform_location allows the index for subroutine functions
to be explicitly set in the shader.

This patch reduces the restriction on the index qualifier in
validate_layout_qualifiers() to allow it to be applied to subroutines
and adds the new subroutine qualifier validation to ast_function::hir().

ast_fully_specified_type::has_qualifiers() is updated to allow the
index qualifier on subroutine functions when explicit uniform locations
is available.

A new check is added to ast_type_qualifier::merge_qualifier() to stop
multiple function qualifiers from being defied, before this patch this
would cause a segfault.

Finally a new variable is added to ir_function_signature to store the
index. This value is validated and the non explicit values assigned in
link_assign_subroutine_types().
---
  src/glsl/ast.h |  2 +-
  src/glsl/ast_to_hir.cpp| 34 --
  src/glsl/ast_type.cpp  | 14 +-
  src/glsl/ir.cpp|  1 +
  src/glsl/ir.h  |  2 ++
  src/glsl/ir_clone.cpp  |  1 +
  src/glsl/linker.cpp| 33 +
  src/mesa/main/mtypes.h |  1 +
  src/mesa/main/shader_query.cpp |  7 +++
  9 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/src/glsl/ast.h b/src/glsl/ast.h
index bfeab6b..1e4a998 100644
--- a/src/glsl/ast.h
+++ b/src/glsl/ast.h
@@ -772,7 +772,7 @@ public:
  class ast_fully_specified_type : public ast_node {
  public:
 virtual void print(void) const;
-   bool has_qualifiers() const;
+   bool has_qualifiers(_mesa_glsl_parse_state *state) const;

 ast_fully_specified_type() : qualifier(), specifier(NULL)
 {
diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
index 6a3ec44..6c56829 100644
--- a/src/glsl/ast_to_hir.cpp
+++ b/src/glsl/ast_to_hir.cpp
@@ -2848,6 +2848,13 @@ apply_explicit_location(const struct ast_type_qualifier 
*qual,
   break;
}

+  /* Check if index was set for the uniform instead of the function */
+  if (qual->flags.q.explicit_index && qual->flags.q.subroutine) {
+ _mesa_glsl_error(loc, state, "an index qualifier can only be "
+  "used with subroutine functions");
+ return;
+  }
+
unsigned qual_index;
if (qual->flags.q.explicit_index &&
process_qualifier_constant(state, loc, "index", qual->index,
@@ -3067,7 +3074,9 @@ apply_layout_qualifier_to_variable(const struct 
ast_type_qualifier *qual,
 if (qual->flags.q.explicit_location) {
apply_explicit_location(qual, var, state, loc);
 } else if (qual->flags.q.explicit_index) {
-  _mesa_glsl_error(loc, state, "explicit index requires explicit 
location");
+  if (!qual->flags.q.subroutine_def)
+ _mesa_glsl_error(loc, state,
+  "explicit index requires explicit location");
 }

 if (qual->flags.q.explicit_binding) {
@@ -5075,7 +5084,7 @@ ast_function::hir(exec_list *instructions,
 /* From page 56 (page 62 of the PDF) of the GLSL 1.30 spec:
  * "No qualifier is allowed on the return type of a function."
  */
-   if (this->return_type->has_qualifiers()) {
+   if (this->return_type->has_qualifiers(state)) {
YYLTYPE loc = this->get_location();
_mesa_glsl_error(& loc, state,
 "function `%s' return type has qualifiers", name);
@@ -5207,6 +5216,27 @@ ast_function::hir(exec_list *instructions,
 if (this->return_type->qualifier.flags.q.subroutine_def) {
int idx;

+  if (this->return_type->qualifier.flags.q.explicit_index) {
+ unsigned qual_index;
+ if (process_qualifier_constant(state, , "index",
+this->return_type->qualifier.index,
+_index)) {
+if (!state->has_explicit_uniform_location()) {
+   _mesa_glsl_error(, state, "subroutine index requires "
+"GL_ARB_explicit_uniform_location or "
+"GLSL 4.30");
+} else if (qual_index >= MAX_SUBROUTINES) {
+   _mesa_glsl_error(, state,
+"invalid subroutine index (%d) index must "
+"be a number between 0 and "
+"GL_MAX_SUBROUTINES - 1 (%d)", qual_index,
+MAX_SUBROUTINES - 1);
+} else {
+   f->subroutine_index = qual_index;
+}
+ }
+  }
+
f->num_subroutine_types = 
this->return_type->qualifier.subroutine_list->declarations.length();
f->subroutine_types = ralloc_array(state, const struct glsl_type *,

Re: [Mesa-dev] [PATCH v3 13/14] glsl: add subroutine index qualifier support

2015-11-19 Thread Timothy Arceri 
Hi Dave/Tapani,

This is the last patch in this series unreviewed is either of you able to
review is as you guys implemented the respective extensions.

Thanks,
Tim
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 13/14] glsl: add subroutine index qualifier support

2015-11-19 Thread Timothy Arceri 
On Fri, 2015-11-20 at 14:25 +1100, Timothy Arceri wrote:
> Hi Dave/Tapani,
> 
> This is the last patch in this series unreviewed is either of you able to
> review is as you guys implemented the respective extensions.

Forgot to say that there are piglit compile tests in master under
 ARB_explicit_uniform_location

and an unreviewed patch that test the correct values are returned by progrm
query

http://patchwork.freedesktop.org/patch/63795/

> 
> Thanks,
> Tim
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v3 13/14] glsl: add subroutine index qualifier support

2015-11-14 Thread Timothy Arceri 
From: Timothy Arceri 

ARB_explicit_uniform_location allows the index for subroutine functions
to be explicitly set in the shader.

This patch reduces the restriction on the index qualifier in
validate_layout_qualifiers() to allow it to be applied to subroutines
and adds the new subroutine qualifier validation to ast_function::hir().

ast_fully_specified_type::has_qualifiers() is updated to allow the
index qualifier on subroutine functions when explicit uniform locations
is available.

A new check is added to ast_type_qualifier::merge_qualifier() to stop
multiple function qualifiers from being defied, before this patch this
would cause a segfault.

Finally a new variable is added to ir_function_signature to store the
index. This value is validated and the non explicit values assigned in
link_assign_subroutine_types().
---
 src/glsl/ast.h |  2 +-
 src/glsl/ast_to_hir.cpp| 34 --
 src/glsl/ast_type.cpp  | 14 +-
 src/glsl/ir.cpp|  1 +
 src/glsl/ir.h  |  2 ++
 src/glsl/ir_clone.cpp  |  1 +
 src/glsl/linker.cpp| 33 +
 src/mesa/main/mtypes.h |  1 +
 src/mesa/main/shader_query.cpp |  7 +++
 9 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/src/glsl/ast.h b/src/glsl/ast.h
index bfeab6b..1e4a998 100644
--- a/src/glsl/ast.h
+++ b/src/glsl/ast.h
@@ -772,7 +772,7 @@ public:
 class ast_fully_specified_type : public ast_node {
 public:
virtual void print(void) const;
-   bool has_qualifiers() const;
+   bool has_qualifiers(_mesa_glsl_parse_state *state) const;
 
ast_fully_specified_type() : qualifier(), specifier(NULL)
{
diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
index 6a3ec44..6c56829 100644
--- a/src/glsl/ast_to_hir.cpp
+++ b/src/glsl/ast_to_hir.cpp
@@ -2848,6 +2848,13 @@ apply_explicit_location(const struct ast_type_qualifier 
*qual,
  break;
   }
 
+  /* Check if index was set for the uniform instead of the function */
+  if (qual->flags.q.explicit_index && qual->flags.q.subroutine) {
+ _mesa_glsl_error(loc, state, "an index qualifier can only be "
+  "used with subroutine functions");
+ return;
+  }
+
   unsigned qual_index;
   if (qual->flags.q.explicit_index &&
   process_qualifier_constant(state, loc, "index", qual->index,
@@ -3067,7 +3074,9 @@ apply_layout_qualifier_to_variable(const struct 
ast_type_qualifier *qual,
if (qual->flags.q.explicit_location) {
   apply_explicit_location(qual, var, state, loc);
} else if (qual->flags.q.explicit_index) {
-  _mesa_glsl_error(loc, state, "explicit index requires explicit 
location");
+  if (!qual->flags.q.subroutine_def)
+ _mesa_glsl_error(loc, state,
+  "explicit index requires explicit location");
}
 
if (qual->flags.q.explicit_binding) {
@@ -5075,7 +5084,7 @@ ast_function::hir(exec_list *instructions,
/* From page 56 (page 62 of the PDF) of the GLSL 1.30 spec:
 * "No qualifier is allowed on the return type of a function."
 */
-   if (this->return_type->has_qualifiers()) {
+   if (this->return_type->has_qualifiers(state)) {
   YYLTYPE loc = this->get_location();
   _mesa_glsl_error(& loc, state,
"function `%s' return type has qualifiers", name);
@@ -5207,6 +5216,27 @@ ast_function::hir(exec_list *instructions,
if (this->return_type->qualifier.flags.q.subroutine_def) {
   int idx;
 
+  if (this->return_type->qualifier.flags.q.explicit_index) {
+ unsigned qual_index;
+ if (process_qualifier_constant(state, , "index",
+this->return_type->qualifier.index,
+_index)) {
+if (!state->has_explicit_uniform_location()) {
+   _mesa_glsl_error(, state, "subroutine index requires "
+"GL_ARB_explicit_uniform_location or "
+"GLSL 4.30");
+} else if (qual_index >= MAX_SUBROUTINES) {
+   _mesa_glsl_error(, state,
+"invalid subroutine index (%d) index must "
+"be a number between 0 and "
+"GL_MAX_SUBROUTINES - 1 (%d)", qual_index,
+MAX_SUBROUTINES - 1);
+} else {
+   f->subroutine_index = qual_index;
+}
+ }
+  }
+
   f->num_subroutine_types = 
this->return_type->qualifier.subroutine_list->declarations.length();
   f->subroutine_types = ralloc_array(state, const struct glsl_type *,
  f->num_subroutine_types);
diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
index 1e89a9e..03ed4dc 100644
--- a/src/glsl/ast_type.cpp
+++