Re: [Mesa-dev] [PATCH 4/4] linker: Accurately track gl_uniform_block::stageref

[Kenneth Graunke]

On Monday, November 7, 2016 9:50:41 PM PST Ian Romanick wrote:
> From: Ian Romanick 
> 
> As the linked per-stage shaders are processed, mark any block that has a
> field that is accessed as referenced.  When combining all the linked
> shaders, combine the per-stage stageref masks.
> 
> This fixes a number of GLES CTS tests including
> ESEXT-CTS.geometry_shader.program_resource.program_resource.  However,
> it makes quite a few more fail.  I have diagnosed the failures, but I'm
> not sure whether we or the tests are wrong.  After optimizations are
> applied, all of the tests are of the form:
> 
> buffer X {
> float f;
> } x;
> 
> void main()
> {
> x.f = x.f;
> }
> 
> The test then queries that x is referenced by that shader stage.  We
> eliminate the assignment of x.f to itself, and that removes the last
> reference to x.  We report that x is not referenced, and the test fails.
> I do not know whether or not we are allowed to eliminate that assignment
> of x.f to itself.
> 
> Signed-off-by: Ian Romanick 

Thanks for fixing all of this, Ian!

Series is:
Reviewed-by: Kenneth Graunke 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] linker: Accurately track gl_uniform_block::stageref

[Nicolai Hähnle]

On 08.11.2016 06:50, Ian Romanick wrote:

From: Ian Romanick 

As the linked per-stage shaders are processed, mark any block that has a
field that is accessed as referenced.  When combining all the linked
shaders, combine the per-stage stageref masks.

This fixes a number of GLES CTS tests including
ESEXT-CTS.geometry_shader.program_resource.program_resource.  However,
it makes quite a few more fail.  I have diagnosed the failures, but I'm
not sure whether we or the tests are wrong.  After optimizations are
applied, all of the tests are of the form:

buffer X {
float f;
} x;

void main()
{
x.f = x.f;
}

The test then queries that x is referenced by that shader stage.  We
eliminate the assignment of x.f to itself, and that removes the last
reference to x.  We report that x is not referenced, and the test fails.
I do not know whether or not we are allowed to eliminate that assignment
of x.f to itself.


Thank you for tackling this -- I've just tried to ignore it for now. 
FWIW, I expect that this also affects the GL CTS in a similar way.


Eliminating the assignment ought to be perfectly fine. The only 
exception I can think of would be if the variable (or block) had a 
volatile qualifier, because the GLSL spec says that "when a volatile 
variable is written, its value must be written to the underlying 
memory". None of the other memory types have similar language.


So I'd suggest changing the test to include a volatile qualifier, and 
possibly fixing the optimization we're doing to honor that. But it would 
be good to clarify with Khronos.


With or without Ilia's suggestion, the patches are

Reviewed-by: Nicolai Hähnle 



Signed-off-by: Ian Romanick 
---
 src/compiler/glsl/link_uniforms.cpp | 65 +
 src/compiler/glsl/linker.cpp|  3 +-
 2 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/src/compiler/glsl/link_uniforms.cpp 
b/src/compiler/glsl/link_uniforms.cpp
index d70614f..29cd24c 100644
--- a/src/compiler/glsl/link_uniforms.cpp
+++ b/src/compiler/glsl/link_uniforms.cpp
@@ -28,6 +28,7 @@
 #include "glsl_symbol_table.h"
 #include "program.h"
 #include "util/string_to_uint_map.h"
+#include "ir_variable_refcount.h"

 /**
  * \file link_uniforms.cpp
@@ -877,6 +878,15 @@ public:
unsigned shader_shadow_samplers;
 };

+static bool
+variable_is_referenced(ir_variable_refcount_visitor , ir_variable *var)
+{
+   ir_variable_refcount_entry *const entry = v.get_variable_entry(var);
+
+   return entry->referenced_count > 0;
+
+}
+
 /**
  * Walks the IR and update the references to uniform blocks in the
  * ir_variables to point at linked shader's list (previously, they
@@ -884,8 +894,13 @@ public:
  * shaders).
  */
 static void
-link_update_uniform_buffer_variables(struct gl_linked_shader *shader)
+link_update_uniform_buffer_variables(struct gl_linked_shader *shader,
+ unsigned stage)
 {
+   ir_variable_refcount_visitor v;
+
+   v.run(shader->ir);
+
foreach_in_list(ir_instruction, node, shader->ir) {
   ir_variable *const var = node->as_variable();

@@ -895,7 +910,44 @@ link_update_uniform_buffer_variables(struct 
gl_linked_shader *shader)
   assert(var->data.mode == ir_var_uniform ||
  var->data.mode == ir_var_shader_storage);

+  unsigned num_blocks = var->data.mode == ir_var_uniform ?
+ shader->NumUniformBlocks : shader->NumShaderStorageBlocks;
+  struct gl_uniform_block **blks = var->data.mode == ir_var_uniform ?
+ shader->UniformBlocks : shader->ShaderStorageBlocks;
+
   if (var->is_interface_instance()) {
+ if (variable_is_referenced(v, var)) {
+/* Since this is an interface instance, the instance type will be
+ * same as the array-stripped variable type.  If the variable type
+ * is an array, then the block names will be suffixed with [0]
+ * through [n-1].  Unlike for non-interface instances, there will
+ * not be structure types here, so the only name sentinel that we
+ * have to worry about is [.
+ */
+assert(var->type->without_array() == var->get_interface_type());
+const char sentinel = var->type->is_array() ? '[' : '\0';
+
+const ptrdiff_t len = strlen(var->get_interface_type()->name);
+for (unsigned i = 0; i < num_blocks; i++) {
+   const char *const begin = blks[i]->Name;
+   const char *const end = strchr(begin, sentinel);
+
+   if (end == NULL)
+  continue;
+
+   if (len != (end - begin))
+  continue;
+
+   /* Even when a match is found, do not "break" here.  This could
+* be an array of instances, and all elements of the array need
+* to be marked as referenced.
+

[Mesa-dev] [PATCH 4/4] linker: Accurately track gl_uniform_block::stageref

[Ian Romanick]

From: Ian Romanick 

As the linked per-stage shaders are processed, mark any block that has a
field that is accessed as referenced.  When combining all the linked
shaders, combine the per-stage stageref masks.

This fixes a number of GLES CTS tests including
ESEXT-CTS.geometry_shader.program_resource.program_resource.  However,
it makes quite a few more fail.  I have diagnosed the failures, but I'm
not sure whether we or the tests are wrong.  After optimizations are
applied, all of the tests are of the form:

buffer X {
float f;
} x;

void main()
{
x.f = x.f;
}

The test then queries that x is referenced by that shader stage.  We
eliminate the assignment of x.f to itself, and that removes the last
reference to x.  We report that x is not referenced, and the test fails.
I do not know whether or not we are allowed to eliminate that assignment
of x.f to itself.

Signed-off-by: Ian Romanick 
---
 src/compiler/glsl/link_uniforms.cpp | 65 +
 src/compiler/glsl/linker.cpp|  3 +-
 2 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/src/compiler/glsl/link_uniforms.cpp 
b/src/compiler/glsl/link_uniforms.cpp
index d70614f..29cd24c 100644
--- a/src/compiler/glsl/link_uniforms.cpp
+++ b/src/compiler/glsl/link_uniforms.cpp
@@ -28,6 +28,7 @@
 #include "glsl_symbol_table.h"
 #include "program.h"
 #include "util/string_to_uint_map.h"
+#include "ir_variable_refcount.h"
 
 /**
  * \file link_uniforms.cpp
@@ -877,6 +878,15 @@ public:
unsigned shader_shadow_samplers;
 };
 
+static bool
+variable_is_referenced(ir_variable_refcount_visitor , ir_variable *var)
+{
+   ir_variable_refcount_entry *const entry = v.get_variable_entry(var);
+
+   return entry->referenced_count > 0;
+
+}
+
 /**
  * Walks the IR and update the references to uniform blocks in the
  * ir_variables to point at linked shader's list (previously, they
@@ -884,8 +894,13 @@ public:
  * shaders).
  */
 static void
-link_update_uniform_buffer_variables(struct gl_linked_shader *shader)
+link_update_uniform_buffer_variables(struct gl_linked_shader *shader,
+ unsigned stage)
 {
+   ir_variable_refcount_visitor v;
+
+   v.run(shader->ir);
+
foreach_in_list(ir_instruction, node, shader->ir) {
   ir_variable *const var = node->as_variable();
 
@@ -895,7 +910,44 @@ link_update_uniform_buffer_variables(struct 
gl_linked_shader *shader)
   assert(var->data.mode == ir_var_uniform ||
  var->data.mode == ir_var_shader_storage);
 
+  unsigned num_blocks = var->data.mode == ir_var_uniform ?
+ shader->NumUniformBlocks : shader->NumShaderStorageBlocks;
+  struct gl_uniform_block **blks = var->data.mode == ir_var_uniform ?
+ shader->UniformBlocks : shader->ShaderStorageBlocks;
+
   if (var->is_interface_instance()) {
+ if (variable_is_referenced(v, var)) {
+/* Since this is an interface instance, the instance type will be
+ * same as the array-stripped variable type.  If the variable type
+ * is an array, then the block names will be suffixed with [0]
+ * through [n-1].  Unlike for non-interface instances, there will
+ * not be structure types here, so the only name sentinel that we
+ * have to worry about is [.
+ */
+assert(var->type->without_array() == var->get_interface_type());
+const char sentinel = var->type->is_array() ? '[' : '\0';
+
+const ptrdiff_t len = strlen(var->get_interface_type()->name);
+for (unsigned i = 0; i < num_blocks; i++) {
+   const char *const begin = blks[i]->Name;
+   const char *const end = strchr(begin, sentinel);
+
+   if (end == NULL)
+  continue;
+
+   if (len != (end - begin))
+  continue;
+
+   /* Even when a match is found, do not "break" here.  This could
+* be an array of instances, and all elements of the array need
+* to be marked as referenced.
+*/
+   if (strncmp(begin, var->get_interface_type()->name, len) == 0) {
+  blks[i]->stageref |= 1U << stage;
+   }
+}
+ }
+
  var->data.location = 0;
  continue;
   }
@@ -910,11 +962,6 @@ link_update_uniform_buffer_variables(struct 
gl_linked_shader *shader)
  sentinel = '[';
   }
 
-  unsigned num_blocks = var->data.mode == ir_var_uniform ?
- shader->NumUniformBlocks : shader->NumShaderStorageBlocks;
-  struct gl_uniform_block **blks = var->data.mode == ir_var_uniform ?
- shader->UniformBlocks : shader->ShaderStorageBlocks;
-
   const unsigned l = strlen(var->name);
   for (unsigned i = 0; i < num_blocks; i++) {
  for (unsigned j = 0; j < blks[i]->NumUniforms;