Re: [Mesa-dev] [PATCH 01/18] nir/linker: handle uniforms without explicit location
On 30/06/18 16:05, Timothy Arceri wrote:
On 30/06/18 00:28, Alejandro Piñeiro wrote:
ARB_gl_spirv points that uniforms in general need explicit
location. But there are still some cases of uniforms without location,
like for example uniform atomic counters. Those doesn't have a
location from the OpenGL point of view (they are identified with a
binding), but Mesa internally assigns it a location.
Signed-off-by: Eduardo Lima
Signed-off-by: Alejandro Piñeiro
Signed-off-by: Neil Roberts
---
The @FIXME included on the patch below is solved with the follow-up
path "nir/linker: use empty block info to assign uniform locations",
so perhaps it makes sense to just squash both patches. I don't have a
strong opinion on that, but I think that it would be easier to review
as splitted patches.
src/compiler/glsl/gl_nir_link_uniforms.c | 61
++--
1 file changed, 59 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c
b/src/compiler/glsl/gl_nir_link_uniforms.c
index c6961fbb6ca..388c1ab63fc 100644
--- a/src/compiler/glsl/gl_nir_link_uniforms.c
+++ b/src/compiler/glsl/gl_nir_link_uniforms.c
@@ -36,6 +36,8 @@
* normal uniforms as mandatory, and so on).
*/
+#define UNMAPPED_UNIFORM_LOC ~0u
+
static void
nir_setup_uniform_remap_tables(struct gl_context *ctx,
struct gl_shader_program *prog)
@@ -58,8 +60,59 @@ nir_setup_uniform_remap_tables(struct gl_context *ctx,
for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
struct gl_uniform_storage *uniform =
>data->UniformStorage[i];
+ if (prog->data->UniformStorage[i].remap_location ==
UNMAPPED_UNIFORM_LOC)
+ continue;
+
+ /* How many new entries for this uniform? */
+ const unsigned entries = MAX2(1, uniform->array_elements);
+ unsigned num_slots = glsl_get_component_slots(uniform->type);
+
+ uniform->storage = [data_pos];
+
+ /* Set remap table entries point to correct gl_uniform_storage. */
+ for (unsigned j = 0; j < entries; j++) {
+ unsigned element_loc = uniform->remap_location + j;
+ prog->UniformRemapTable[element_loc] = uniform;
+
+ data_pos += num_slots;
+ }
+ }
+
+ /* Reserve locations for rest of the uniforms. */
+ for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
+ struct gl_uniform_storage *uniform =
>data->UniformStorage[i];
+
+ if (uniform->is_shader_storage)
+ continue;
+
+ /* Built-in uniforms should not get any location. */
+ if (uniform->builtin)
+ continue;
+
+ /* Explicit ones have been set already. */
+ if (uniform->remap_location != UNMAPPED_UNIFORM_LOC)
+ continue;
+
/* How many new entries for this uniform? */
const unsigned entries = MAX2(1, uniform->array_elements);
+
+ /* @FIXME: By now, we add un-assigned uniform locations to the
end of
+ * the uniform file. We need to keep track of empty locations
and use
+ * them.
+ */
Maybe reword this to:
/* @FIXME: For now, we add un-assigned uniform locations to the end of
* the uniform file. We should fix this to keep track of empty
* locations and use those first.
*/
+ unsigned chosen_location = prog->NumUniformRemapTable;
Can we please just rename chosen_location
Whoops that should be:
Can we please just rename chosen_location -> location
With those two nits this patch is:
Reviewed-by: Timothy Arceri
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 01/18] nir/linker: handle uniforms without explicit location
On 30/06/18 00:28, Alejandro Piñeiro wrote:
ARB_gl_spirv points that uniforms in general need explicit
location. But there are still some cases of uniforms without location,
like for example uniform atomic counters. Those doesn't have a
location from the OpenGL point of view (they are identified with a
binding), but Mesa internally assigns it a location.
Signed-off-by: Eduardo Lima
Signed-off-by: Alejandro Piñeiro
Signed-off-by: Neil Roberts
---
The @FIXME included on the patch below is solved with the follow-up
path "nir/linker: use empty block info to assign uniform locations",
so perhaps it makes sense to just squash both patches. I don't have a
strong opinion on that, but I think that it would be easier to review
as splitted patches.
src/compiler/glsl/gl_nir_link_uniforms.c | 61 ++--
1 file changed, 59 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c
b/src/compiler/glsl/gl_nir_link_uniforms.c
index c6961fbb6ca..388c1ab63fc 100644
--- a/src/compiler/glsl/gl_nir_link_uniforms.c
+++ b/src/compiler/glsl/gl_nir_link_uniforms.c
@@ -36,6 +36,8 @@
* normal uniforms as mandatory, and so on).
*/
+#define UNMAPPED_UNIFORM_LOC ~0u
+
static void
nir_setup_uniform_remap_tables(struct gl_context *ctx,
struct gl_shader_program *prog)
@@ -58,8 +60,59 @@ nir_setup_uniform_remap_tables(struct gl_context *ctx,
for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
struct gl_uniform_storage *uniform = >data->UniformStorage[i];
+ if (prog->data->UniformStorage[i].remap_location == UNMAPPED_UNIFORM_LOC)
+ continue;
+
+ /* How many new entries for this uniform? */
+ const unsigned entries = MAX2(1, uniform->array_elements);
+ unsigned num_slots = glsl_get_component_slots(uniform->type);
+
+ uniform->storage = [data_pos];
+
+ /* Set remap table entries point to correct gl_uniform_storage. */
+ for (unsigned j = 0; j < entries; j++) {
+ unsigned element_loc = uniform->remap_location + j;
+ prog->UniformRemapTable[element_loc] = uniform;
+
+ data_pos += num_slots;
+ }
+ }
+
+ /* Reserve locations for rest of the uniforms. */
+ for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
+ struct gl_uniform_storage *uniform = >data->UniformStorage[i];
+
+ if (uniform->is_shader_storage)
+ continue;
+
+ /* Built-in uniforms should not get any location. */
+ if (uniform->builtin)
+ continue;
+
+ /* Explicit ones have been set already. */
+ if (uniform->remap_location != UNMAPPED_UNIFORM_LOC)
+ continue;
+
/* How many new entries for this uniform? */
const unsigned entries = MAX2(1, uniform->array_elements);
+
+ /* @FIXME: By now, we add un-assigned uniform locations to the end of
+ * the uniform file. We need to keep track of empty locations and use
+ * them.
+ */
Maybe reword this to:
/* @FIXME: For now, we add un-assigned uniform locations to the end of
* the uniform file. We should fix this to keep track of empty
* locations and use those first.
*/
+ unsigned chosen_location = prog->NumUniformRemapTable;
Can we please just rename chosen_location
With those two nits this patch is:
Reviewed-by: Timothy Arceri
+
+ /* resize remap table to fit new entries */
+ prog->UniformRemapTable =
+ reralloc(prog,
+ prog->UniformRemapTable,
+ struct gl_uniform_storage *,
+ prog->NumUniformRemapTable + entries);
+ prog->NumUniformRemapTable += entries;
+
+ /* set the base location in remap table for the uniform */
+ uniform->remap_location = chosen_location;
+
unsigned num_slots = glsl_get_component_slots(uniform->type);
uniform->storage = [data_pos];
@@ -302,8 +355,12 @@ nir_link_uniform(struct gl_context *ctx,
}
uniform->active_shader_mask |= 1 << stage;
- /* Uniform has an explicit location */
- uniform->remap_location = location;
+ if (location >= 0) {
+ /* Uniform has an explicit location */
+ uniform->remap_location = location;
+ } else {
+ uniform->remap_location = UNMAPPED_UNIFORM_LOC;
+ }
/* @FIXME: the initialization of the following will be done as we
* implement support for their specific features, like SSBO, atomics,
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 01/18] nir/linker: handle uniforms without explicit location
ARB_gl_spirv points that uniforms in general need explicit
location. But there are still some cases of uniforms without location,
like for example uniform atomic counters. Those doesn't have a
location from the OpenGL point of view (they are identified with a
binding), but Mesa internally assigns it a location.
Signed-off-by: Eduardo Lima
Signed-off-by: Alejandro Piñeiro
Signed-off-by: Neil Roberts
---
The @FIXME included on the patch below is solved with the follow-up
path "nir/linker: use empty block info to assign uniform locations",
so perhaps it makes sense to just squash both patches. I don't have a
strong opinion on that, but I think that it would be easier to review
as splitted patches.
src/compiler/glsl/gl_nir_link_uniforms.c | 61 ++--
1 file changed, 59 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c
b/src/compiler/glsl/gl_nir_link_uniforms.c
index c6961fbb6ca..388c1ab63fc 100644
--- a/src/compiler/glsl/gl_nir_link_uniforms.c
+++ b/src/compiler/glsl/gl_nir_link_uniforms.c
@@ -36,6 +36,8 @@
* normal uniforms as mandatory, and so on).
*/
+#define UNMAPPED_UNIFORM_LOC ~0u
+
static void
nir_setup_uniform_remap_tables(struct gl_context *ctx,
struct gl_shader_program *prog)
@@ -58,8 +60,59 @@ nir_setup_uniform_remap_tables(struct gl_context *ctx,
for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
struct gl_uniform_storage *uniform = >data->UniformStorage[i];
+ if (prog->data->UniformStorage[i].remap_location == UNMAPPED_UNIFORM_LOC)
+ continue;
+
+ /* How many new entries for this uniform? */
+ const unsigned entries = MAX2(1, uniform->array_elements);
+ unsigned num_slots = glsl_get_component_slots(uniform->type);
+
+ uniform->storage = [data_pos];
+
+ /* Set remap table entries point to correct gl_uniform_storage. */
+ for (unsigned j = 0; j < entries; j++) {
+ unsigned element_loc = uniform->remap_location + j;
+ prog->UniformRemapTable[element_loc] = uniform;
+
+ data_pos += num_slots;
+ }
+ }
+
+ /* Reserve locations for rest of the uniforms. */
+ for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) {
+ struct gl_uniform_storage *uniform = >data->UniformStorage[i];
+
+ if (uniform->is_shader_storage)
+ continue;
+
+ /* Built-in uniforms should not get any location. */
+ if (uniform->builtin)
+ continue;
+
+ /* Explicit ones have been set already. */
+ if (uniform->remap_location != UNMAPPED_UNIFORM_LOC)
+ continue;
+
/* How many new entries for this uniform? */
const unsigned entries = MAX2(1, uniform->array_elements);
+
+ /* @FIXME: By now, we add un-assigned uniform locations to the end of
+ * the uniform file. We need to keep track of empty locations and use
+ * them.
+ */
+ unsigned chosen_location = prog->NumUniformRemapTable;
+
+ /* resize remap table to fit new entries */
+ prog->UniformRemapTable =
+ reralloc(prog,
+ prog->UniformRemapTable,
+ struct gl_uniform_storage *,
+ prog->NumUniformRemapTable + entries);
+ prog->NumUniformRemapTable += entries;
+
+ /* set the base location in remap table for the uniform */
+ uniform->remap_location = chosen_location;
+
unsigned num_slots = glsl_get_component_slots(uniform->type);
uniform->storage = [data_pos];
@@ -302,8 +355,12 @@ nir_link_uniform(struct gl_context *ctx,
}
uniform->active_shader_mask |= 1 << stage;
- /* Uniform has an explicit location */
- uniform->remap_location = location;
+ if (location >= 0) {
+ /* Uniform has an explicit location */
+ uniform->remap_location = location;
+ } else {
+ uniform->remap_location = UNMAPPED_UNIFORM_LOC;
+ }
/* @FIXME: the initialization of the following will be done as we
* implement support for their specific features, like SSBO, atomics,
--
2.14.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
