[Mesa-dev] [PATCH] anv: fix bug when using component qualifier in FS outputs

[Samuel Iglesias Gonsálvez]

On Wed, 2017-11-08 at 08:19 -0800, Jason Ekstrand wrote:
> On Thu, Nov 2, 2017 at 12:28 AM, Samuel Iglesias Gonsálvez <
> siglesias at igalia.com> wrote:
> 
> > We can write to the same output but in different components, like
> > in this example:
> > 
> > layout(location = 0, component = 0) out ivec2 dEQP_FragColor_0;
> > layout(location = 0, component = 2) out ivec2 dEQP_FragColor_1;
> > 
> > Therefore, they are not two different outputs but only one.
> > 
> > Fixes:
> > 
> > dEQP-VK.glsl.440.linkage.varying.component.frag_out.*
> > 
> > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> > ---
> >  src/compiler/shader_enums.h     |  1 +
> >  src/intel/vulkan/anv_pipeline.c | 10 +++++++++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/compiler/shader_enums.h
> > b/src/compiler/shader_enums.h
> > index 9d229d4199e..90729dbfd96 100644
> > --- a/src/compiler/shader_enums.h
> > +++ b/src/compiler/shader_enums.h
> > @@ -603,6 +603,7 @@ typedef enum
> >     FRAG_RESULT_DATA5,
> >     FRAG_RESULT_DATA6,
> >     FRAG_RESULT_DATA7,
> > +   FRAG_RESULT_MAX, /**< Number of fragment program results */
> >  } gl_frag_result;
> > 
> >  const char *gl_frag_result_name(gl_frag_result result);
> > diff --git a/src/intel/vulkan/anv_pipeline.c
> > b/src/intel/vulkan/anv_
> > pipeline.c
> > index 907b24a758d..be007f24e3f 100644
> > --- a/src/intel/vulkan/anv_pipeline.c
> > +++ b/src/intel/vulkan/anv_pipeline.c
> > @@ -872,6 +872,8 @@ anv_pipeline_compile_fs(struct anv_pipeline
> > *pipeline,
> >        unsigned num_rts = 0;
> >        struct anv_pipeline_binding rt_bindings[8];
> >        nir_function_impl *impl = nir_shader_get_entrypoint(nir);
> > +      int map_old_to_new_loc[FRAG_RESULT_MAX];
> > +      memset(map_old_to_new_loc, -1, sizeof(int) *
> > FRAG_RESULT_MAX);
> >        nir_foreach_variable_safe(var, &nir->outputs) {
> >           if (var->data.location < FRAG_RESULT_DATA0)
> >              continue;
> > @@ -886,7 +888,13 @@ anv_pipeline_compile_fs(struct anv_pipeline
> > *pipeline,
> >           }
> > 
> >           /* Give it a new, compacted, location */
> > -         var->data.location = FRAG_RESULT_DATA0 + num_rts;
> > +         if (var->data.location != -1) {
> > 
> 
> How is this even possible?
> 
> 
> > +            if (map_old_to_new_loc[var->data.location] == -1)
> > +               map_old_to_new_loc[var->data.location] =
> > FRAG_RESULT_DATA0 + num_rts;
> > +            var->data.location = map_old_to_new_loc[var-
> > >data.location];
> > +         } else {
> > +            var->data.location = FRAG_RESULT_DATA0 + num_rts;
> > +         }
> > 
> 
> I see what you are trying to fix.  However, if we're going to get
> composite
> types right, I think we need to do it in three passes:
> 
>  1) Define a `bool rt_used[MAX_RTS]` map and walk over everything and
> flag
> the given RT as used.
>  2) Walk over rt_used and generated a `int rt_to_binding[MAX_RTS]`
> map as
> well as set up the `rt_bindings` map (which is really just a map
> going in
> the other direction).
>  3) Walk the variables again and re-assign locations using
> `rt_to_binding`
> 

I wrote a patch without step 1) as I need to walk the variables for
`rt_bindings` map because of the array lenght.

I am going to send the patch now so you can comment about it.

Sam

> I don't see how any more direct method will actually yield the
> correct
> result in all of the crazy cases such as where you have an array of
> vec2s
> at component 0 and then a bunch of stray floats at components 2 and
> 3.
> 
> Sorry for not responding earlier,
> --Jason
> 
> 
> >           unsigned array_len =
> >              glsl_type_is_array(var->type) ? glsl_get_length(var-
> > >type) :
> > 1;
> > --
> > 2.14.2
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >