On Oct 15, 2015 15:17, "Kenneth Graunke" <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> 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: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a> > Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > --- > 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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>