[Mesa-dev] [PATCH 3/3] i965: Fix scalar VS float[] and vec2[] output arrays.

[Jason Ekstrand]

On Oct 15, 2015 15:17, "Kenneth Graunke" <kenneth at whitecape.org> wrote:
>
> The scalar VS backend has never handled float[] and vec2[] outputs
> correctly (my original code was broken).  Outputs need to be padded
> out to vec4 slots.
>
> In fs_visitor::nir_setup_outputs(), we tried to process each vec4 slot
> by looping from 0 to ALIGN(type_size_scalar(type), 4) / 4.  However,
> this is wrong: type_size_scalar() for a float[2] would return 2, or
> for vec2[2] it would return 4.  This looked like a single slot, even
> though in reality each array element would be stored in separate vec4
> slots.

I thought that looked fishy.

> Because of this bug, outputs[] and output_components[] would not get
> initialized for the second element's VARYING_SLOT, which meant
> emit_urb_writes() would skip writing them.  Nothing used those values,
> and dead code elimination threw a party.

Heh.

> The new type_size_4x() function pads array elements correctly, but
> still counts in scalar components, generating correct indices in
> store_output intrinsics.
>
> Not observed to fix any Piglit or dEQP tests, but does fix various
> tcs-input Piglit tests on a branch that implements tessellation shaders.
>
> Cc: mesa-stable at lists.freedesktop.org
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 2 +-
>  src/mesa/drivers/dri/i965/brw_nir.c      | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 0e044d0..a290656 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -93,7 +93,7 @@ fs_visitor::nir_setup_outputs()
>
>        switch (stage) {
>        case MESA_SHADER_VERTEX:
> -         for (unsigned int i = 0; i < ALIGN(type_size_scalar(var->type),
4) / 4; i++) {
> +         for (int i = 0; i < type_size_4x(var->type) / 4; i++) {

Why not just type_size_vec4?  I know it makes it match what's below but as
long as we define type_size_4x nearby and as Connor said in the other
patch, it should he fairly obvious.

For that matter we could just make it a static function defined right above
nir_setup_outputs.  If that's the case, probably just squash it into this
patch?

>              int output = var->data.location + i;
>              this->outputs[output] = offset(reg, bld, 4 * i);
>              this->output_components[output] = vector_elements;
> diff --git a/src/mesa/drivers/dri/i965/brw_nir.c
b/src/mesa/drivers/dri/i965/brw_nir.c
> index 1b4dace..c7f94a6 100644
> --- a/src/mesa/drivers/dri/i965/brw_nir.c
> +++ b/src/mesa/drivers/dri/i965/brw_nir.c
> @@ -117,7 +117,8 @@ brw_nir_lower_outputs(nir_shader *nir, bool is_scalar)
>     case MESA_SHADER_GEOMETRY:
>        if (is_scalar) {
>           nir_assign_var_locations(&nir->outputs, &nir->num_outputs,
> -                                  type_size_scalar);
> +                                  type_size_4x);
> +         nir_lower_io(nir, nir_var_shader_out, type_size_4x);
>        } else {
>           nir_foreach_variable(var, &nir->outputs)
>              var->data.driver_location = var->data.location;
> --
> 2.6.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151015/84528860/attachment.html>