[Mesa-dev] [PATCH 4/4] glsl: Fix gl_program::OutputsWritten computation for dual-source blending.

[Ilia Mirkin]

Series is

Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>

Thanks for teasing these issues apart.

On Tue, Aug 30, 2016 at 6:17 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> In the fragment shader OutputsWritten is a bitset of FRAG_RESULT_*
> enumerants, which represent the location of each color output written
> by the shader.  The secondary and primary color outputs of a given
> render target using dual-source blending have the same location, so
> the 'idx' computation below will give the wrong bit as result if the
> 'var->data.index' term is non-zero -- E.g. if the shader writes the
> primary and secondary colors of the FRAG_RESULT_COLOR output,
> ir_set_program_inouts will think that the shader writes both
> FRAG_RESULT_COLOR and FRAG_RESULT_SAMPLE_MASK, which is just bogus.
>
> That would cause the brw_wm_prog_key::nr_color_regions computation
> done in the i965 driver during fragment shader precompilation to be
> wrong, which currently leads to unnecessary recompilation of shaders
> that use dual-source blending, and triggers an assertion failure in
> fs_visitor::emit_fb_writes() on my i965-fb-fetch branch.
> ---
>  src/compiler/glsl/ir_set_program_inouts.cpp | 2 +-
>  src/mesa/state_tracker/st_program.c         | 4 +---
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/src/compiler/glsl/ir_set_program_inouts.cpp b/src/compiler/glsl/ir_set_program_inouts.cpp
> index 06d9973..4f6c886 100644
> --- a/src/compiler/glsl/ir_set_program_inouts.cpp
> +++ b/src/compiler/glsl/ir_set_program_inouts.cpp
> @@ -96,7 +96,7 @@ mark(struct gl_program *prog, ir_variable *var, int offset, int len,
>     for (int i = 0; i < len; i++) {
>        assert(var->data.location != -1);
>
> -      int idx = var->data.location + var->data.index + offset + i;
> +      int idx = var->data.location + offset + i;
>        bool is_patch_generic = var->data.patch &&
>                                idx != VARYING_SLOT_TESS_LEVEL_INNER &&
>                                idx != VARYING_SLOT_TESS_LEVEL_OUTER;
> diff --git a/src/mesa/state_tracker/st_program.c b/src/mesa/state_tracker/st_program.c
> index 429d0c9..2a4edfa 100644
> --- a/src/mesa/state_tracker/st_program.c
> +++ b/src/mesa/state_tracker/st_program.c
> @@ -586,9 +586,7 @@ bool
>  st_translate_fragment_program(struct st_context *st,
>                                struct st_fragment_program *stfp)
>  {
> -   GLuint outputMapping[2 * FRAG_RESULT_MAX] =
> -      { 0 /* XXX - Avoid temporary regression due to bogus OutputsWritten
> -           *       bitset. */ };
> +   GLuint outputMapping[2 * FRAG_RESULT_MAX];
>     GLuint inputMapping[VARYING_SLOT_MAX];
>     GLuint inputSlotToAttr[VARYING_SLOT_MAX];
>     GLuint interpMode[PIPE_MAX_SHADER_INPUTS];  /* XXX size? */
> --
> 2.9.0
>