Discussion:
[PATCH 2/2] st/mesa: fix CTS regression caused by fcbb93e86024
Add Reply
Samuel Pitoiset
2017-08-01 08:07:19 UTC
Reply
Permalink
Raw Message
Don't you think it's just safer to revert the bad commit for 17.2 and
fix it later on?
When generation the storage offset for struct members we need
to skip opaque types as they no longer have backing storage.
Fixes: fcbb93e86024 ("mesa: stop assigning unused storage for non-bindless opaque types")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101983
---
src/mesa/Makefile.sources | 2 +
src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 16 ++++-
src/mesa/state_tracker/st_glsl_types.cpp | 105 +++++++++++++++++++++++++++++
src/mesa/state_tracker/st_glsl_types.h | 44 ++++++++++++
4 files changed, 165 insertions(+), 2 deletions(-)
create mode 100644 src/mesa/state_tracker/st_glsl_types.cpp
create mode 100644 src/mesa/state_tracker/st_glsl_types.h
diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
index 97c8287acb..2c557c3952 100644
--- a/src/mesa/Makefile.sources
+++ b/src/mesa/Makefile.sources
@@ -500,20 +500,22 @@ STATETRACKER_FILES = \
state_tracker/st_extensions.c \
state_tracker/st_extensions.h \
state_tracker/st_format.c \
state_tracker/st_format.h \
state_tracker/st_gen_mipmap.c \
state_tracker/st_gen_mipmap.h \
state_tracker/st_gl_api.h \
state_tracker/st_glsl_to_nir.cpp \
state_tracker/st_glsl_to_tgsi.cpp \
state_tracker/st_glsl_to_tgsi.h \
+- state_tracker/st_glsl_types.cpp \
+- state_tracker/st_glsl_types.h \
state_tracker/st_manager.c \
state_tracker/st_manager.h \
state_tracker/st_mesa_to_tgsi.c \
state_tracker/st_mesa_to_tgsi.h \
state_tracker/st_nir.h \
state_tracker/st_nir_lower_builtin.c \
state_tracker/st_nir_lower_tex_src_plane.c \
state_tracker/st_pbo.c \
state_tracker/st_pbo.h \
state_tracker/st_program.c \
diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
index 2fca398a51..54a749d388 100644
--- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
@@ -42,20 +42,21 @@
#include "main/shaderapi.h"
#include "main/shaderimage.h"
#include "program/prog_instruction.h"
#include "pipe/p_context.h"
#include "pipe/p_screen.h"
#include "tgsi/tgsi_ureg.h"
#include "tgsi/tgsi_info.h"
#include "util/u_math.h"
#include "util/u_memory.h"
+#include "st_glsl_types.h"
#include "st_program.h"
#include "st_mesa_to_tgsi.h"
#include "st_format.h"
#include "st_nir.h"
#include "st_shader_cache.h"
#include "util/hash_table.h"
#include <algorithm>
#define PROGRAM_ANY_CONST ((1 << PROGRAM_STATE_VAR) | \
@@ -2825,22 +2826,30 @@ shrink_array_declarations(struct inout_decl *decls, unsigned count,
*usage_mask |= BITFIELD64_BIT(decl->mesa_index + j);
}
}
}
void
glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
{
ir_constant *index;
st_src_reg src;
- int element_size = type_size(ir->type);
bool is_2D = false;
+ ir_variable *var = ir->variable_referenced();
+
+ /* We only need the logic provided by st_glsl_storage_type_size()
+ * for arrays of structs. Indirect sampler and image indexing is handled
+ * elsewhere.
+ */
+ int element_size = ir->type->without_array()->is_record() ?
+ type_size(ir->type);
index = ir->array_index->constant_expression_value();
ir->array->accept(this);
src = this->result;
if (ir->array->ir_type != ir_type_dereference_array) {
switch (this->prog->Target) {
is_2D = (src.file == PROGRAM_INPUT || src.file == PROGRAM_OUTPUT) &&
@@ -2916,28 +2925,31 @@ glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
src.type = ir->type->base_type;
this->result = src;
}
void
glsl_to_tgsi_visitor::visit(ir_dereference_record *ir)
{
unsigned int i;
const glsl_type *struct_type = ir->record->type;
+ ir_variable *var = ir->record->variable_referenced();
int offset = 0;
ir->record->accept(this);
+ assert(var);
for (i = 0; i < struct_type->length; i++) {
if (strcmp(struct_type->fields.structure[i].name, ir->field) == 0)
break;
- offset += type_size(struct_type->fields.structure[i].type);
+ const glsl_type *member_type = struct_type->fields.structure[i].type;
+ offset += st_glsl_storage_type_size(member_type, var->data.bindless);
}
/* If the type is smaller than a vec4, replicate the last channel out. */
if (ir->type->is_scalar() || ir->type->is_vector())
this->result.swizzle = swizzle_for_size(ir->type->vector_elements);
else
this->result.swizzle = SWIZZLE_NOOP;
this->result.index += offset;
this->result.type = ir->type->base_type;
diff --git a/src/mesa/state_tracker/st_glsl_types.cpp b/src/mesa/state_tracker/st_glsl_types.cpp
new file mode 100644
index 0000000000..50936025d9
--- /dev/null
+++ b/src/mesa/state_tracker/st_glsl_types.cpp
@@ -0,0 +1,105 @@
+/*
+ * Copyright (C) 2005-2007 Brian Paul All Rights Reserved.
+ * Copyright (C) 2008 VMware, Inc. All Rights Reserved.
+ * Copyright © 2010 Intel Corporation
+ * Copyright © 2011 Bryan Cain
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include "st_glsl_types.h"
+
+/**
+ * Returns the number of places to offset the uniform index, given the type of
+ * a struct member. We use this because samplers and images have backing
+ * storeage only when they are bindless.
+ */
+int
+st_glsl_storage_type_size(const struct glsl_type *type, bool is_bindless)
+{
+ unsigned int i;
+ int size;
+
+ switch (type->base_type) {
+ if (type->is_matrix()) {
+ return type->matrix_columns;
+ } else {
+ /* Regardless of size of vector, it gets a vec4. This is bad
+ * packing for things like floats, but otherwise arrays become a
+ * mess. Hopefully a later pass over the code can pack scalars
+ * down if appropriate.
+ */
+ return 1;
+ }
+ break;
+ if (type->is_matrix()) {
+ if (type->vector_elements <= 2)
+ return type->matrix_columns;
+ else
+ return type->matrix_columns * 2;
+ } else {
+ /* For doubles if we have a double or dvec2 they fit in one
+ * vec4, else they need 2 vec4s.
+ */
+ if (type->vector_elements <= 2)
+ return 1;
+ else
+ return 2;
+ }
+ break;
+ if (type->vector_elements <= 2)
+ return 1;
+ else
+ return 2;
+ assert(type->length > 0);
+ return st_glsl_storage_type_size(type->fields.array, is_bindless) *
+ type->length;
+ size = 0;
+ for (i = 0; i < type->length; i++) {
+ size += st_glsl_storage_type_size(type->fields.structure[i].type,
+ is_bindless);
+ }
+ return size;
+ if (!is_bindless)
+ return 0;
+ /* fall through */
+ return 1;
+ assert(!"Invalid type in type_size");
+ break;
+ }
+ return 0;
+}
diff --git a/src/mesa/state_tracker/st_glsl_types.h b/src/mesa/state_tracker/st_glsl_types.h
new file mode 100644
index 0000000000..915816d1fa
--- /dev/null
+++ b/src/mesa/state_tracker/st_glsl_types.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2005-2007 Brian Paul All Rights Reserved.
+ * Copyright (C) 2008 VMware, Inc. All Rights Reserved.
+ * Copyright © 2010 Intel Corporation
+ * Copyright © 2011 Bryan Cain
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __ST_GLSL_TYPES_H__
+#define __ST_GLSL_TYPES_H__
+
+#include "compiler/glsl_types.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+int st_glsl_storage_type_size(const struct glsl_type *type,
+ bool is_bindless);
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __ST_GLSL_TYPES_H__ */
Timothy Arceri
2017-08-01 09:37:01 UTC
Reply
Permalink
Raw Message
Post by Samuel Pitoiset
Don't you think it's just safer to revert the bad commit for 17.2 and
fix it later on?
I'm not overly worried. If you really want to go that way we can, but I
don't think it's necessary.
Post by Samuel Pitoiset
When generation the storage offset for struct members we need
to skip opaque types as they no longer have backing storage.
Fixes: fcbb93e86024 ("mesa: stop assigning unused storage for
non-bindless opaque types")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101983
---
src/mesa/Makefile.sources | 2 +
src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 16 ++++-
src/mesa/state_tracker/st_glsl_types.cpp | 105
+++++++++++++++++++++++++++++
src/mesa/state_tracker/st_glsl_types.h | 44 ++++++++++++
4 files changed, 165 insertions(+), 2 deletions(-)
create mode 100644 src/mesa/state_tracker/st_glsl_types.cpp
create mode 100644 src/mesa/state_tracker/st_glsl_types.h
diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
index 97c8287acb..2c557c3952 100644
--- a/src/mesa/Makefile.sources
+++ b/src/mesa/Makefile.sources
@@ -500,20 +500,22 @@ STATETRACKER_FILES = \
state_tracker/st_extensions.c \
state_tracker/st_extensions.h \
state_tracker/st_format.c \
state_tracker/st_format.h \
state_tracker/st_gen_mipmap.c \
state_tracker/st_gen_mipmap.h \
state_tracker/st_gl_api.h \
state_tracker/st_glsl_to_nir.cpp \
state_tracker/st_glsl_to_tgsi.cpp \
state_tracker/st_glsl_to_tgsi.h \
+- state_tracker/st_glsl_types.cpp \
+- state_tracker/st_glsl_types.h \
state_tracker/st_manager.c \
state_tracker/st_manager.h \
state_tracker/st_mesa_to_tgsi.c \
state_tracker/st_mesa_to_tgsi.h \
state_tracker/st_nir.h \
state_tracker/st_nir_lower_builtin.c \
state_tracker/st_nir_lower_tex_src_plane.c \
state_tracker/st_pbo.c \
state_tracker/st_pbo.h \
state_tracker/st_program.c \
diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
index 2fca398a51..54a749d388 100644
--- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
@@ -42,20 +42,21 @@
#include "main/shaderapi.h"
#include "main/shaderimage.h"
#include "program/prog_instruction.h"
#include "pipe/p_context.h"
#include "pipe/p_screen.h"
#include "tgsi/tgsi_ureg.h"
#include "tgsi/tgsi_info.h"
#include "util/u_math.h"
#include "util/u_memory.h"
+#include "st_glsl_types.h"
#include "st_program.h"
#include "st_mesa_to_tgsi.h"
#include "st_format.h"
#include "st_nir.h"
#include "st_shader_cache.h"
#include "util/hash_table.h"
#include <algorithm>
#define PROGRAM_ANY_CONST ((1 << PROGRAM_STATE_VAR) | \
@@ -2825,22 +2826,30 @@ shrink_array_declarations(struct inout_decl
*decls, unsigned count,
*usage_mask |= BITFIELD64_BIT(decl->mesa_index + j);
}
}
}
void
glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
{
ir_constant *index;
st_src_reg src;
- int element_size = type_size(ir->type);
bool is_2D = false;
+ ir_variable *var = ir->variable_referenced();
+
+ /* We only need the logic provided by st_glsl_storage_type_size()
+ * for arrays of structs. Indirect sampler and image indexing is handled
+ * elsewhere.
+ */
+ int element_size = ir->type->without_array()->is_record() ?
+ type_size(ir->type);
index = ir->array_index->constant_expression_value();
ir->array->accept(this);
src = this->result;
if (ir->array->ir_type != ir_type_dereference_array) {
switch (this->prog->Target) {
is_2D = (src.file == PROGRAM_INPUT || src.file ==
PROGRAM_OUTPUT) &&
@@ -2916,28 +2925,31 @@
glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
src.type = ir->type->base_type;
this->result = src;
}
void
glsl_to_tgsi_visitor::visit(ir_dereference_record *ir)
{
unsigned int i;
const glsl_type *struct_type = ir->record->type;
+ ir_variable *var = ir->record->variable_referenced();
int offset = 0;
ir->record->accept(this);
+ assert(var);
for (i = 0; i < struct_type->length; i++) {
if (strcmp(struct_type->fields.structure[i].name, ir->field) == 0)
break;
- offset += type_size(struct_type->fields.structure[i].type);
+ const glsl_type *member_type =
struct_type->fields.structure[i].type;
+ offset += st_glsl_storage_type_size(member_type,
var->data.bindless);
}
/* If the type is smaller than a vec4, replicate the last channel out. */
if (ir->type->is_scalar() || ir->type->is_vector())
this->result.swizzle =
swizzle_for_size(ir->type->vector_elements);
else
this->result.swizzle = SWIZZLE_NOOP;
this->result.index += offset;
this->result.type = ir->type->base_type;
diff --git a/src/mesa/state_tracker/st_glsl_types.cpp
b/src/mesa/state_tracker/st_glsl_types.cpp
new file mode 100644
index 0000000000..50936025d9
--- /dev/null
+++ b/src/mesa/state_tracker/st_glsl_types.cpp
@@ -0,0 +1,105 @@
+/*
+ * Copyright (C) 2005-2007 Brian Paul All Rights Reserved.
+ * Copyright (C) 2008 VMware, Inc. All Rights Reserved.
+ * Copyright © 2010 Intel Corporation
+ * Copyright © 2011 Bryan Cain
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include "st_glsl_types.h"
+
+/**
+ * Returns the number of places to offset the uniform index, given the type of
+ * a struct member. We use this because samplers and images have backing
+ * storeage only when they are bindless.
+ */
+int
+st_glsl_storage_type_size(const struct glsl_type *type, bool
is_bindless)
+{
+ unsigned int i;
+ int size;
+
+ switch (type->base_type) {
+ if (type->is_matrix()) {
+ return type->matrix_columns;
+ } else {
+ /* Regardless of size of vector, it gets a vec4. This is bad
+ * packing for things like floats, but otherwise arrays become a
+ * mess. Hopefully a later pass over the code can pack scalars
+ * down if appropriate.
+ */
+ return 1;
+ }
+ break;
+ if (type->is_matrix()) {
+ if (type->vector_elements <= 2)
+ return type->matrix_columns;
+ else
+ return type->matrix_columns * 2;
+ } else {
+ /* For doubles if we have a double or dvec2 they fit in one
+ * vec4, else they need 2 vec4s.
+ */
+ if (type->vector_elements <= 2)
+ return 1;
+ else
+ return 2;
+ }
+ break;
+ if (type->vector_elements <= 2)
+ return 1;
+ else
+ return 2;
+ assert(type->length > 0);
+ return st_glsl_storage_type_size(type->fields.array,
is_bindless) *
+ type->length;
+ size = 0;
+ for (i = 0; i < type->length; i++) {
+ size +=
st_glsl_storage_type_size(type->fields.structure[i].type,
+ is_bindless);
+ }
+ return size;
+ if (!is_bindless)
+ return 0;
+ /* fall through */
+ return 1;
+ assert(!"Invalid type in type_size");
+ break;
+ }
+ return 0;
+}
diff --git a/src/mesa/state_tracker/st_glsl_types.h
b/src/mesa/state_tracker/st_glsl_types.h
new file mode 100644
index 0000000000..915816d1fa
--- /dev/null
+++ b/src/mesa/state_tracker/st_glsl_types.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2005-2007 Brian Paul All Rights Reserved.
+ * Copyright (C) 2008 VMware, Inc. All Rights Reserved.
+ * Copyright © 2010 Intel Corporation
+ * Copyright © 2011 Bryan Cain
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __ST_GLSL_TYPES_H__
+#define __ST_GLSL_TYPES_H__
+
+#include "compiler/glsl_types.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+int st_glsl_storage_type_size(const struct glsl_type *type,
+ bool is_bindless);
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __ST_GLSL_TYPES_H__ */
Timothy Arceri
2017-08-08 05:34:07 UTC
Reply
Permalink
Raw Message
Post by Timothy Arceri
Post by Samuel Pitoiset
Don't you think it's just safer to revert the bad commit for 17.2 and
fix it later on?
I'm not overly worried. If you really want to go that way we can, but I
don't think it's necessary.
So how do we move forward on this? Can we have the revert applied to
17.2 and commit this to master?

Ccing Emil for answers :)
Post by Timothy Arceri
Post by Samuel Pitoiset
When generation the storage offset for struct members we need
to skip opaque types as they no longer have backing storage.
Fixes: fcbb93e86024 ("mesa: stop assigning unused storage for
non-bindless opaque types")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101983
---
src/mesa/Makefile.sources | 2 +
src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 16 ++++-
src/mesa/state_tracker/st_glsl_types.cpp | 105
+++++++++++++++++++++++++++++
src/mesa/state_tracker/st_glsl_types.h | 44 ++++++++++++
4 files changed, 165 insertions(+), 2 deletions(-)
create mode 100644 src/mesa/state_tracker/st_glsl_types.cpp
create mode 100644 src/mesa/state_tracker/st_glsl_types.h
diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
index 97c8287acb..2c557c3952 100644
--- a/src/mesa/Makefile.sources
+++ b/src/mesa/Makefile.sources
@@ -500,20 +500,22 @@ STATETRACKER_FILES = \
state_tracker/st_extensions.c \
state_tracker/st_extensions.h \
state_tracker/st_format.c \
state_tracker/st_format.h \
state_tracker/st_gen_mipmap.c \
state_tracker/st_gen_mipmap.h \
state_tracker/st_gl_api.h \
state_tracker/st_glsl_to_nir.cpp \
state_tracker/st_glsl_to_tgsi.cpp \
state_tracker/st_glsl_to_tgsi.h \
+- state_tracker/st_glsl_types.cpp \
+- state_tracker/st_glsl_types.h \
state_tracker/st_manager.c \
state_tracker/st_manager.h \
state_tracker/st_mesa_to_tgsi.c \
state_tracker/st_mesa_to_tgsi.h \
state_tracker/st_nir.h \
state_tracker/st_nir_lower_builtin.c \
state_tracker/st_nir_lower_tex_src_plane.c \
state_tracker/st_pbo.c \
state_tracker/st_pbo.h \
state_tracker/st_program.c \
diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
index 2fca398a51..54a749d388 100644
--- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
@@ -42,20 +42,21 @@
#include "main/shaderapi.h"
#include "main/shaderimage.h"
#include "program/prog_instruction.h"
#include "pipe/p_context.h"
#include "pipe/p_screen.h"
#include "tgsi/tgsi_ureg.h"
#include "tgsi/tgsi_info.h"
#include "util/u_math.h"
#include "util/u_memory.h"
+#include "st_glsl_types.h"
#include "st_program.h"
#include "st_mesa_to_tgsi.h"
#include "st_format.h"
#include "st_nir.h"
#include "st_shader_cache.h"
#include "util/hash_table.h"
#include <algorithm>
#define PROGRAM_ANY_CONST ((1 << PROGRAM_STATE_VAR) | \
@@ -2825,22 +2826,30 @@ shrink_array_declarations(struct inout_decl
*decls, unsigned count,
*usage_mask |= BITFIELD64_BIT(decl->mesa_index + j);
}
}
}
void
glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
{
ir_constant *index;
st_src_reg src;
- int element_size = type_size(ir->type);
bool is_2D = false;
+ ir_variable *var = ir->variable_referenced();
+
+ /* We only need the logic provided by st_glsl_storage_type_size()
+ * for arrays of structs. Indirect sampler and image indexing is handled
+ * elsewhere.
+ */
+ int element_size = ir->type->without_array()->is_record() ?
+ type_size(ir->type);
index = ir->array_index->constant_expression_value();
ir->array->accept(this);
src = this->result;
if (ir->array->ir_type != ir_type_dereference_array) {
switch (this->prog->Target) {
is_2D = (src.file == PROGRAM_INPUT || src.file ==
PROGRAM_OUTPUT) &&
@@ -2916,28 +2925,31 @@
glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
src.type = ir->type->base_type;
this->result = src;
}
void
glsl_to_tgsi_visitor::visit(ir_dereference_record *ir)
{
unsigned int i;
const glsl_type *struct_type = ir->record->type;
+ ir_variable *var = ir->record->variable_referenced();
int offset = 0;
ir->record->accept(this);
+ assert(var);
for (i = 0; i < struct_type->length; i++) {
if (strcmp(struct_type->fields.structure[i].name, ir->field) == 0)
break;
- offset += type_size(struct_type->fields.structure[i].type);
+ const glsl_type *member_type =
struct_type->fields.structure[i].type;
+ offset += st_glsl_storage_type_size(member_type,
var->data.bindless);
}
/* If the type is smaller than a vec4, replicate the last channel out. */
if (ir->type->is_scalar() || ir->type->is_vector())
this->result.swizzle =
swizzle_for_size(ir->type->vector_elements);
else
this->result.swizzle = SWIZZLE_NOOP;
this->result.index += offset;
this->result.type = ir->type->base_type;
diff --git a/src/mesa/state_tracker/st_glsl_types.cpp
b/src/mesa/state_tracker/st_glsl_types.cpp
new file mode 100644
index 0000000000..50936025d9
--- /dev/null
+++ b/src/mesa/state_tracker/st_glsl_types.cpp
@@ -0,0 +1,105 @@
+/*
+ * Copyright (C) 2005-2007 Brian Paul All Rights Reserved.
+ * Copyright (C) 2008 VMware, Inc. All Rights Reserved.
+ * Copyright © 2010 Intel Corporation
+ * Copyright © 2011 Bryan Cain
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include "st_glsl_types.h"
+
+/**
+ * Returns the number of places to offset the uniform index, given the type of
+ * a struct member. We use this because samplers and images have backing
+ * storeage only when they are bindless.
+ */
+int
+st_glsl_storage_type_size(const struct glsl_type *type, bool is_bindless)
+{
+ unsigned int i;
+ int size;
+
+ switch (type->base_type) {
+ if (type->is_matrix()) {
+ return type->matrix_columns;
+ } else {
+ /* Regardless of size of vector, it gets a vec4. This is bad
+ * packing for things like floats, but otherwise arrays become a
+ * mess. Hopefully a later pass over the code can pack scalars
+ * down if appropriate.
+ */
+ return 1;
+ }
+ break;
+ if (type->is_matrix()) {
+ if (type->vector_elements <= 2)
+ return type->matrix_columns;
+ else
+ return type->matrix_columns * 2;
+ } else {
+ /* For doubles if we have a double or dvec2 they fit in one
+ * vec4, else they need 2 vec4s.
+ */
+ if (type->vector_elements <= 2)
+ return 1;
+ else
+ return 2;
+ }
+ break;
+ if (type->vector_elements <= 2)
+ return 1;
+ else
+ return 2;
+ assert(type->length > 0);
+ return st_glsl_storage_type_size(type->fields.array,
is_bindless) *
+ type->length;
+ size = 0;
+ for (i = 0; i < type->length; i++) {
+ size +=
st_glsl_storage_type_size(type->fields.structure[i].type,
+ is_bindless);
+ }
+ return size;
+ if (!is_bindless)
+ return 0;
+ /* fall through */
+ return 1;
+ assert(!"Invalid type in type_size");
+ break;
+ }
+ return 0;
+}
diff --git a/src/mesa/state_tracker/st_glsl_types.h
b/src/mesa/state_tracker/st_glsl_types.h
new file mode 100644
index 0000000000..915816d1fa
--- /dev/null
+++ b/src/mesa/state_tracker/st_glsl_types.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2005-2007 Brian Paul All Rights Reserved.
+ * Copyright (C) 2008 VMware, Inc. All Rights Reserved.
+ * Copyright © 2010 Intel Corporation
+ * Copyright © 2011 Bryan Cain
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __ST_GLSL_TYPES_H__
+#define __ST_GLSL_TYPES_H__
+
+#include "compiler/glsl_types.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+int st_glsl_storage_type_size(const struct glsl_type *type,
+ bool is_bindless);
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __ST_GLSL_TYPES_H__ */
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Samuel Pitoiset
2017-08-08 09:21:53 UTC
Reply
Permalink
Raw Message
Post by Timothy Arceri
Post by Timothy Arceri
Post by Samuel Pitoiset
Don't you think it's just safer to revert the bad commit for 17.2 and
fix it later on?
I'm not overly worried. If you really want to go that way we can, but
I don't think it's necessary.
So how do we move forward on this? Can we have the revert applied to
17.2 and commit this to master?
Sorry, missed that. I would prefer to revert yes, but opinions are
welcome. :-)
Post by Timothy Arceri
Ccing Emil for answers :)
Post by Timothy Arceri
Post by Samuel Pitoiset
When generation the storage offset for struct members we need
to skip opaque types as they no longer have backing storage.
Fixes: fcbb93e86024 ("mesa: stop assigning unused storage for
non-bindless opaque types")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101983
---
src/mesa/Makefile.sources | 2 +
src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 16 ++++-
src/mesa/state_tracker/st_glsl_types.cpp | 105
+++++++++++++++++++++++++++++
src/mesa/state_tracker/st_glsl_types.h | 44 ++++++++++++
4 files changed, 165 insertions(+), 2 deletions(-)
create mode 100644 src/mesa/state_tracker/st_glsl_types.cpp
create mode 100644 src/mesa/state_tracker/st_glsl_types.h
diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
index 97c8287acb..2c557c3952 100644
--- a/src/mesa/Makefile.sources
+++ b/src/mesa/Makefile.sources
@@ -500,20 +500,22 @@ STATETRACKER_FILES = \
state_tracker/st_extensions.c \
state_tracker/st_extensions.h \
state_tracker/st_format.c \
state_tracker/st_format.h \
state_tracker/st_gen_mipmap.c \
state_tracker/st_gen_mipmap.h \
state_tracker/st_gl_api.h \
state_tracker/st_glsl_to_nir.cpp \
state_tracker/st_glsl_to_tgsi.cpp \
state_tracker/st_glsl_to_tgsi.h \
+- state_tracker/st_glsl_types.cpp \
+- state_tracker/st_glsl_types.h \
state_tracker/st_manager.c \
state_tracker/st_manager.h \
state_tracker/st_mesa_to_tgsi.c \
state_tracker/st_mesa_to_tgsi.h \
state_tracker/st_nir.h \
state_tracker/st_nir_lower_builtin.c \
state_tracker/st_nir_lower_tex_src_plane.c \
state_tracker/st_pbo.c \
state_tracker/st_pbo.h \
state_tracker/st_program.c \
diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
index 2fca398a51..54a749d388 100644
--- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
@@ -42,20 +42,21 @@
#include "main/shaderapi.h"
#include "main/shaderimage.h"
#include "program/prog_instruction.h"
#include "pipe/p_context.h"
#include "pipe/p_screen.h"
#include "tgsi/tgsi_ureg.h"
#include "tgsi/tgsi_info.h"
#include "util/u_math.h"
#include "util/u_memory.h"
+#include "st_glsl_types.h"
#include "st_program.h"
#include "st_mesa_to_tgsi.h"
#include "st_format.h"
#include "st_nir.h"
#include "st_shader_cache.h"
#include "util/hash_table.h"
#include <algorithm>
#define PROGRAM_ANY_CONST ((1 << PROGRAM_STATE_VAR) | \
@@ -2825,22 +2826,30 @@ shrink_array_declarations(struct inout_decl
*decls, unsigned count,
*usage_mask |= BITFIELD64_BIT(decl->mesa_index + j);
}
}
}
void
glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
{
ir_constant *index;
st_src_reg src;
- int element_size = type_size(ir->type);
bool is_2D = false;
+ ir_variable *var = ir->variable_referenced();
+
+ /* We only need the logic provided by st_glsl_storage_type_size()
+ * for arrays of structs. Indirect sampler and image indexing is handled
+ * elsewhere.
+ */
+ int element_size = ir->type->without_array()->is_record() ?
+ type_size(ir->type);
index = ir->array_index->constant_expression_value();
ir->array->accept(this);
src = this->result;
if (ir->array->ir_type != ir_type_dereference_array) {
switch (this->prog->Target) {
is_2D = (src.file == PROGRAM_INPUT || src.file == PROGRAM_OUTPUT) &&
@@ -2916,28 +2925,31 @@
glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
src.type = ir->type->base_type;
this->result = src;
}
void
glsl_to_tgsi_visitor::visit(ir_dereference_record *ir)
{
unsigned int i;
const glsl_type *struct_type = ir->record->type;
+ ir_variable *var = ir->record->variable_referenced();
int offset = 0;
ir->record->accept(this);
+ assert(var);
for (i = 0; i < struct_type->length; i++) {
if (strcmp(struct_type->fields.structure[i].name, ir->field) == 0)
break;
- offset += type_size(struct_type->fields.structure[i].type);
+ const glsl_type *member_type =
struct_type->fields.structure[i].type;
+ offset += st_glsl_storage_type_size(member_type,
var->data.bindless);
}
/* If the type is smaller than a vec4, replicate the last channel out. */
if (ir->type->is_scalar() || ir->type->is_vector())
this->result.swizzle =
swizzle_for_size(ir->type->vector_elements);
else
this->result.swizzle = SWIZZLE_NOOP;
this->result.index += offset;
this->result.type = ir->type->base_type;
diff --git a/src/mesa/state_tracker/st_glsl_types.cpp
b/src/mesa/state_tracker/st_glsl_types.cpp
new file mode 100644
index 0000000000..50936025d9
--- /dev/null
+++ b/src/mesa/state_tracker/st_glsl_types.cpp
@@ -0,0 +1,105 @@
+/*
+ * Copyright (C) 2005-2007 Brian Paul All Rights Reserved.
+ * Copyright (C) 2008 VMware, Inc. All Rights Reserved.
+ * Copyright © 2010 Intel Corporation
+ * Copyright © 2011 Bryan Cain
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include "st_glsl_types.h"
+
+/**
+ * Returns the number of places to offset the uniform index, given the type of
+ * a struct member. We use this because samplers and images have backing
+ * storeage only when they are bindless.
+ */
+int
+st_glsl_storage_type_size(const struct glsl_type *type, bool is_bindless)
+{
+ unsigned int i;
+ int size;
+
+ switch (type->base_type) {
+ if (type->is_matrix()) {
+ return type->matrix_columns;
+ } else {
+ /* Regardless of size of vector, it gets a vec4. This is bad
+ * packing for things like floats, but otherwise arrays become a
+ * mess. Hopefully a later pass over the code can pack scalars
+ * down if appropriate.
+ */
+ return 1;
+ }
+ break;
+ if (type->is_matrix()) {
+ if (type->vector_elements <= 2)
+ return type->matrix_columns;
+ else
+ return type->matrix_columns * 2;
+ } else {
+ /* For doubles if we have a double or dvec2 they fit in one
+ * vec4, else they need 2 vec4s.
+ */
+ if (type->vector_elements <= 2)
+ return 1;
+ else
+ return 2;
+ }
+ break;
+ if (type->vector_elements <= 2)
+ return 1;
+ else
+ return 2;
+ assert(type->length > 0);
+ return st_glsl_storage_type_size(type->fields.array,
is_bindless) *
+ type->length;
+ size = 0;
+ for (i = 0; i < type->length; i++) {
+ size +=
st_glsl_storage_type_size(type->fields.structure[i].type,
+ is_bindless);
+ }
+ return size;
+ if (!is_bindless)
+ return 0;
+ /* fall through */
+ return 1;
+ assert(!"Invalid type in type_size");
+ break;
+ }
+ return 0;
+}
diff --git a/src/mesa/state_tracker/st_glsl_types.h
b/src/mesa/state_tracker/st_glsl_types.h
new file mode 100644
index 0000000000..915816d1fa
--- /dev/null
+++ b/src/mesa/state_tracker/st_glsl_types.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2005-2007 Brian Paul All Rights Reserved.
+ * Copyright (C) 2008 VMware, Inc. All Rights Reserved.
+ * Copyright © 2010 Intel Corporation
+ * Copyright © 2011 Bryan Cain
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __ST_GLSL_TYPES_H__
+#define __ST_GLSL_TYPES_H__
+
+#include "compiler/glsl_types.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+int st_glsl_storage_type_size(const struct glsl_type *type,
+ bool is_bindless);
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __ST_GLSL_TYPES_H__ */
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Emil Velikov
2017-08-08 10:15:11 UTC
Reply
Permalink
Raw Message
Post by Timothy Arceri
Post by Samuel Pitoiset
Don't you think it's just safer to revert the bad commit for 17.2 and
fix it later on?
I'm not overly worried. If you really want to go that way we can, but I
don't think it's necessary.
So how do we move forward on this? Can we have the revert applied to 17.2
and commit this to master?
Sorry, missed that. I would prefer to revert yes, but opinions are welcome.
:-)
Thanks guys.

I think we're fine to have the revert for 17.2 and this series in master.
When you send the revert please note that it's 17.2 only, and ideally
reference the [comprehensive] fixes in master.

Couple of suggestions:
- the "+-" in the Makefile seems bogus, I think it should be "+" only?
- s/When generation the/When generating the/ in the summary
- reword the commit title to say what it does - "correctly calculate
the storage offset" ?

-Emil
Emil Velikov
2017-08-11 20:32:56 UTC
Reply
Permalink
Raw Message
When generation the storage offset for struct members we need
to skip opaque types as they no longer have backing storage.
Fixes: fcbb93e86024 ("mesa: stop assigning unused storage for non-bindless opaque types")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101983
Tim, did you went with another solution or the patches are waiting for review?
I was about to pick the revert for stable, but would need a reference
for the fix in master.
---
src/mesa/Makefile.sources | 2 +
src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 16 ++++-
src/mesa/state_tracker/st_glsl_types.cpp | 105 +++++++++++++++++++++++++++++
src/mesa/state_tracker/st_glsl_types.h | 44 ++++++++++++
I think the st_glsl_types.* are revert conflicts and should not be needed.

If that's correct, this series is overall smaller and preferable for
stable, since it does not diverge from master.
It's your call though.

Thanks
Emil
Timothy Arceri
2017-08-13 22:51:12 UTC
Reply
Permalink
Raw Message
Post by Emil Velikov
When generation the storage offset for struct members we need
to skip opaque types as they no longer have backing storage.
Fixes: fcbb93e86024 ("mesa: stop assigning unused storage for non-bindless opaque types")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101983
Tim, did you went with another solution or the patches are waiting for review?
I was about to pick the revert for stable, but would need a reference
for the fix in master.
Still waiting on review. I'll resend today with your previous comments
about wording addressed.
Post by Emil Velikov
---
src/mesa/Makefile.sources | 2 +
src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 16 ++++-
src/mesa/state_tracker/st_glsl_types.cpp | 105 +++++++++++++++++++++++++++++
src/mesa/state_tracker/st_glsl_types.h | 44 ++++++++++++
I think the st_glsl_types.* are revert conflicts and should not be needed.
If that's correct, this series is overall smaller and preferable for
stable, since it does not diverge from master.
It's your call though.
I believe the functions I add back with st_glsl_types.* are slightly
different than the ones that were there before. However Samuel would
rather his patch go into stable anyway, I'm fine with that also.
Post by Emil Velikov
Thanks
Emil
Loading...