[Mesa-dev] [PATCH 1/3] nir: fix spelling typo

[Rob Clark]

Signed-off-by: Rob Clark <***@gmail.com>
---
src/compiler/nir/nir_linking_helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c
index a05890ada43..1ab9c095657 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -414,7 +414,7 @@ compact_components(nir_shader *producer, nir_shader *consumer, uint8_t *comps,

/* We ignore complex types above and all other vector types should
* have been split into scalar variables by the lower_io_to_scalar
- * pass. The only exeption should by OpenGL xfb varyings.
+ * pass. The only exception should by OpenGL xfb varyings.
*/
if (glsl_get_vector_elements(type) != 1)
continue;

[Rob Clark]

Not entirely sure when this changed, but it seem like
LinkedTransformFeedback is (usually?) populated, even if
NumVaryings is zero. So make the check about whether it
is safe to nir_compact_varyings() a bit more complete.

Signed-off-by: Rob Clark <***@gmail.com>
---
src/mesa/state_tracker/st_glsl_to_nir.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp b/src/mesa/state_tracker/st_glsl_to_nir.cpp
index d0475fb538a..7406e26e2f8 100644
--- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
@@ -758,7 +758,8 @@ st_link_nir(struct gl_context *ctx,
* the pipe_stream_output->output_register field is based on the
* pre-compacted driver_locations.
*/
- if (!prev_shader->sh.LinkedTransformFeedback)
+ if (!(prev_shader->sh.LinkedTransformFeedback &&
+ prev_shader->sh.LinkedTransformFeedback->NumVarying > 0))
nir_compact_varyings(shader_program->_LinkedShaders[prev]->Program->nir,
nir, ctx->API != API_OPENGL_COMPAT);
}

[Timothy Arceri]

Post by Rob Clark
Not entirely sure when this changed, but it seem like
LinkedTransformFeedback is (usually?) populated,

Yeah it looks like this code was wrong when introduced. I also recall
somebody complaining the performance dropped in Shadow of Mordor with
Eric's fix, which makes a little more sense now.

Looking over the code in link_varying.cpp what happens is we always
create LinkedTransformFeedback for the last vertex stage. Which means we
have not been packing the frgament shader inputs since this fix was
introduced :( Maybe update the commit message to make this a little clearer.

Also please add:

Fixes: dbd52585fa9f ("st/nir: Disable varying packing when doing
transform feedback.")

With those changes patches 1-2 are:

Reviewed-by: Timothy Arceri <***@itsqueeze.com>

Thanks for looking into this.

even if

Post by Rob Clark
NumVaryings is zero. So make the check about whether it
is safe to nir_compact_varyings() a bit more complete.
---
src/mesa/state_tracker/st_glsl_to_nir.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp b/src/mesa/state_tracker/st_glsl_to_nir.cpp
index d0475fb538a..7406e26e2f8 100644
--- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
+++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
@@ -758,7 +758,8 @@ st_link_nir(struct gl_context *ctx,
* the pipe_stream_output->output_register field is based on the
* pre-compacted driver_locations.
*/
- if (!prev_shader->sh.LinkedTransformFeedback)
+ if (!(prev_shader->sh.LinkedTransformFeedback &&
+ prev_shader->sh.LinkedTransformFeedback->NumVarying > 0))
nir_compact_varyings(shader_program->_LinkedShaders[prev]->Program->nir,
nir, ctx->API != API_OPENGL_COMPAT);
}

[Rob Clark]

Previously, if we had a .z or .w component that could be compacted
to .y, we'd could overlook that opportunity.

Signed-off-by: Rob Clark <***@gmail.com>
---
src/compiler/nir/nir_linking_helpers.c | 30 ++++++++++++++++++++------
1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c
index 1ab9c095657..ce368a3c132 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -431,12 +431,30 @@ compact_components(nir_shader *producer, nir_shader *consumer, uint8_t *comps,
uint8_t interp = get_interp_type(var, type, default_to_smooth_interp);
for (; cursor[interp] < 32; cursor[interp]++) {
uint8_t cursor_used_comps = comps[cursor[interp]];
+ uint8_t unused_comps = ~cursor_used_comps;

- /* We couldn't find anywhere to pack the varying continue on. */
- if (cursor[interp] == location &&
- (var->data.location_frac == 0 ||
- cursor_used_comps & ((1 << (var->data.location_frac)) - 1)))
- break;
+ /* Don't search beyond our current location, we are just trying
+ * to pack later varyings to lower positions:
+ */
+ if (cursor[interp] == location) {
+ if (var->data.location_frac == 0)
+ break;
+
+ /* If not already aligned to slot, see if we can shift it up.
+ * Note that if we get this far it is a scalar so we know that
+ * shifting this var to any other open position won't conflict
+ * with it's current position.
+ */
+ unsigned p = ffs(unused_comps & 0xf);
+ if (!p)
+ break;
+
+ /* ffs returns 1 for bit zero: */
+ p--;
+
+ if (p >= var->data.location_frac)
+ break;
+ }

/* We can only pack varyings with matching interpolation types */
if (interp_type[cursor[interp]] != interp)
@@ -460,8 +478,6 @@ compact_components(nir_shader *producer, nir_shader *consumer, uint8_t *comps,
if (!cursor_used_comps)
continue;

- uint8_t unused_comps = ~cursor_used_comps;
-
for (unsigned i = 0; i < 4; i++) {
uint8_t new_var_comps = 1 << i;
if (unused_comps & new_var_comps) {

[Timothy Arceri]

I'd much rather land the first 3 patches from this series if possible.

https://patchwork.freedesktop.org/series/53800/

I've confirmed it packs the shaders you were looking at as expected once
you patch 2 is applied. The series makes this code much more flexible
(for future improvements) and easier to follow.

Post by Rob Clark
Previously, if we had a .z or .w component that could be compacted
to .y, we'd could overlook that opportunity.
---
src/compiler/nir/nir_linking_helpers.c | 30 ++++++++++++++++++++------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c
index 1ab9c095657..ce368a3c132 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -431,12 +431,30 @@ compact_components(nir_shader *producer, nir_shader *consumer, uint8_t *comps,
uint8_t interp = get_interp_type(var, type, default_to_smooth_interp);
for (; cursor[interp] < 32; cursor[interp]++) {
uint8_t cursor_used_comps = comps[cursor[interp]];
+ uint8_t unused_comps = ~cursor_used_comps;
- /* We couldn't find anywhere to pack the varying continue on. */
- if (cursor[interp] == location &&
- (var->data.location_frac == 0 ||
- cursor_used_comps & ((1 << (var->data.location_frac)) - 1)))
- break;
+ /* Don't search beyond our current location, we are just trying
+ */
+ if (cursor[interp] == location) {
+ if (var->data.location_frac == 0)
+ break;
+
+ /* If not already aligned to slot, see if we can shift it up.
+ * Note that if we get this far it is a scalar so we know that
+ * shifting this var to any other open position won't conflict
+ * with it's current position.
+ */
+ unsigned p = ffs(unused_comps & 0xf);
+ if (!p)
+ break;
+
+ /* ffs returns 1 for bit zero: */
+ p--;
+
+ if (p >= var->data.location_frac)
+ break;
+ }
/* We can only pack varyings with matching interpolation types */
if (interp_type[cursor[interp]] != interp)
@@ -460,8 +478,6 @@ compact_components(nir_shader *producer, nir_shader *consumer, uint8_t *comps,
if (!cursor_used_comps)
continue;
- uint8_t unused_comps = ~cursor_used_comps;
-
for (unsigned i = 0; i < 4; i++) {
uint8_t new_var_comps = 1 << i;
if (unused_comps & new_var_comps) {

[Ilia Mirkin]

FWIW you can't rely on things being packed. With something like SSO,
or enhanced layouts, you can get some funky stuff.

That is fine by me. I've come up with a workaround for now (just
adding some dummy output components to handle the .x__w case) so at
least he .w varying doesn't get lost with that shader.
BR,
-R

Post by Timothy Arceri
I'd much rather land the first 3 patches from this series if possible.
https://patchwork.freedesktop.org/series/53800/
I've confirmed it packs the shaders you were looking at as expected once
you patch 2 is applied. The series makes this code much more flexible
(for future improvements) and easier to follow.

Post by Rob Clark
Previously, if we had a .z or .w component that could be compacted
to .y, we'd could overlook that opportunity.
---
src/compiler/nir/nir_linking_helpers.c | 30 ++++++++++++++++++++------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c
index 1ab9c095657..ce368a3c132 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -431,12 +431,30 @@ compact_components(nir_shader *producer, nir_shader *consumer, uint8_t *comps,
uint8_t interp = get_interp_type(var, type, default_to_smooth_interp);
for (; cursor[interp] < 32; cursor[interp]++) {
uint8_t cursor_used_comps = comps[cursor[interp]];
+ uint8_t unused_comps = ~cursor_used_comps;
- /* We couldn't find anywhere to pack the varying continue on. */
- if (cursor[interp] == location &&
- (var->data.location_frac == 0 ||
- cursor_used_comps & ((1 << (var->data.location_frac)) - 1)))
- break;
+ /* Don't search beyond our current location, we are just trying
+ */
+ if (cursor[interp] == location) {
+ if (var->data.location_frac == 0)
+ break;
+
+ /* If not already aligned to slot, see if we can shift it up.
+ * Note that if we get this far it is a scalar so we know that
+ * shifting this var to any other open position won't conflict
+ * with it's current position.
+ */
+ unsigned p = ffs(unused_comps & 0xf);
+ if (!p)
+ break;
+
+ /* ffs returns 1 for bit zero: */
+ p--;
+
+ if (p >= var->data.location_frac)
+ break;
+ }
/* We can only pack varyings with matching interpolation types */
if (interp_type[cursor[interp]] != interp)
@@ -460,8 +478,6 @@ compact_components(nir_shader *producer, nir_shader *consumer, uint8_t *comps,
if (!cursor_used_comps)
continue;
- uint8_t unused_comps = ~cursor_used_comps;
-
for (unsigned i = 0; i < 4; i++) {
uint8_t new_var_comps = 1 << i;
if (unused_comps & new_var_comps) {

_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Jason Ekstrand]

Rb

Post by Rob Clark
---
src/compiler/nir/nir_linking_helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/compiler/nir/nir_linking_helpers.c
b/src/compiler/nir/nir_linking_helpers.c
index a05890ada43..1ab9c095657 100644
--- a/src/compiler/nir/nir_linking_helpers.c
+++ b/src/compiler/nir/nir_linking_helpers.c
@@ -414,7 +414,7 @@ compact_components(nir_shader *producer, nir_shader
*consumer, uint8_t *comps,
/* We ignore complex types above and all other vector types should
* have been split into scalar variables by the lower_io_to_scalar
- * pass. The only exeption should by OpenGL xfb varyings.
+ * pass. The only exception should by OpenGL xfb varyings.
*/
if (glsl_get_vector_elements(type) != 1)
continue;
--
2.19.2
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev