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

[Samuel Iglesias Gonsálvez]

On Tue, 2017-11-28 at 12:49 -0800, Ian Romanick wrote:
> Based on my (weak) understanding things in this part of the code, I
> think this is ok.  There are a couple minor nits below.  With those
> addressed and unless Jason has (time to give) some objections, this
> patch is
> 
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> 
> On 11/28/2017 02:06 AM, Samuel Iglesias Gonsálvez wrote:
> > This patch is still unreviewed.
> > 
> > Sam
> > 
> > On Wed, 2017-11-15 at 07:39 +0100, Samuel Iglesias Gonsálvez 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 | 46
> > > +++++++++++++++++++++++++++--
> > > ------------
> > >  2 files changed, 32 insertions(+), 15 deletions(-)
> > > 
> > > 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 */
> 
> Does this add warnings about enum values not handled in switch
> statements?
> 

I have not seen any warning, but I am going to get rid of this hunk
to avoid any warning in the future.

I am going to do the rest of changes suggested below.

Thanks,

Sam

> > >  } 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..92cfc2898f5 100644
> > > --- a/src/intel/vulkan/anv_pipeline.c
> > > +++ b/src/intel/vulkan/anv_pipeline.c
> > > @@ -870,30 +870,28 @@ anv_pipeline_compile_fs(struct anv_pipeline
> > > *pipeline,
> > >        }
> > >  
> > >        unsigned num_rts = 0;
> > > -      struct anv_pipeline_binding rt_bindings[8];
> > > +      const int max_rt = FRAG_RESULT_MAX - FRAG_RESULT_DATA0;
> > > +      struct anv_pipeline_binding rt_bindings[max_rt];
> > >        nir_function_impl *impl = nir_shader_get_entrypoint(nir);
> > > +      int rt_to_bindings[max_rt];
> > > +      memset(rt_to_bindings, -1, sizeof(int) * max_rt);
> 
> Isn't this just sizeof(rt_to_bindings)?
> 
> > > +
> > > +      /* Set new, compacted, location */
> > >        nir_foreach_variable_safe(var, &nir->outputs) {
> > >           if (var->data.location < FRAG_RESULT_DATA0)
> > >              continue;
> > >  
> > >           unsigned rt = var->data.location - FRAG_RESULT_DATA0;
> > > -         if (rt >= key.nr_color_regions) {
> > > -            /* Out-of-bounds, throw it away */
> > > -            var->data.mode = nir_var_local;
> > > -            exec_node_remove(&var->node);
> > > -            exec_list_push_tail(&impl->locals, &var->node);
> > > +         if (rt_to_bindings[rt] != -1 || rt >=
> > > key.nr_color_regions)
> > >              continue;
> > > -         }
> > > -
> > > -         /* Give it a new, compacted, location */
> > > -         var->data.location = FRAG_RESULT_DATA0 + num_rts;
> > > -
> > > -         unsigned array_len =
> > > +         const unsigned array_len =
> > >              glsl_type_is_array(var->type) ? glsl_get_length(var-
> > > > type) : 1;
> > > 
> > > -         assert(num_rts + array_len <= 8);
> > > +         assert(num_rts + array_len <= max_rt);
> > > +
> > > +         rt_to_bindings[rt] = num_rts;
> > >  
> > >           for (unsigned i = 0; i < array_len; i++) {
> > > -            rt_bindings[num_rts + i] = (struct
> > > anv_pipeline_binding)
> > > {
> > > +            rt_bindings[rt_to_bindings[rt] + i] = (struct
> > > anv_pipeline_binding) {
> > >                 .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
> > >                 .binding = 0,
> > >                 .index = rt + i,
> > > @@ -903,6 +901,24 @@ anv_pipeline_compile_fs(struct anv_pipeline
> > > *pipeline,
> > >           num_rts += array_len;
> > >        }
> > >  
> > > +      nir_foreach_variable_safe(var, &nir->outputs) {
> > > +         if (var->data.location < FRAG_RESULT_DATA0)
> > > +            continue;
> > > +
> > > +         unsigned rt = var->data.location - FRAG_RESULT_DATA0;
> 
> const
> 
> > > +         if (rt >= key.nr_color_regions) {
> > > +            /* Out-of-bounds, throw it away */
> > > +            var->data.mode = nir_var_local;
> > > +            exec_node_remove(&var->node);
> > > +            exec_list_push_tail(&impl->locals, &var->node);
> > > +            continue;
> > > +         }
> > > +
> > > +         /* Give it the new location */
> > > +         assert(rt_to_bindings[rt] != -1);
> > > +         var->data.location = rt_to_bindings[rt] +
> > > FRAG_RESULT_DATA0;
> > > +      }
> > > +
> > >        if (num_rts == 0) {
> > >           /* If we have no render targets, we need a null render
> > > target */
> > >           rt_bindings[0] = (struct anv_pipeline_binding) {
> > > @@ -913,7 +929,7 @@ anv_pipeline_compile_fs(struct anv_pipeline
> > > *pipeline,
> > >           num_rts = 1;
> > >        }
> > >  
> > > -      assert(num_rts <= 8);
> > > +      assert(num_rts <= max_rt);
> > >        map.surface_to_descriptor -= num_rts;
> > >        map.surface_count += num_rts;
> > >        assert(map.surface_count <= 256);
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>