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

[Jason Ekstrand]

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 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171108/b7e045e5/attachment-0001.html>