[Mesa-dev] [PATCH 2/7] nir: don't assert when xfb_buffer/stride is present but not xfb_offset

[Jason Ekstrand]

On Thu, Nov 8, 2018 at 7:22 AM Alejandro Piñeiro <apinheiro at igalia.com>
wrote:

> In order to allow nir_gather_xfb_info to be used on OpenGL,
> specifically ARB_gl_spirv.
>
> So, from OpenGL 4.6 spec, section 11.1.2.1, "Output Variables":
>
>     "outputs specifying both an *XfbBuffer* and an *Offset* are
>      captured, while outputs not specifying both of these are not
>      captured. Values are captured each time the shader writes to such
>      a decorated object."
>
> This implies that are captured if both are present, and not if one of
> those are lacking. Technically, it doesn't explicitly point that
> having just one or the other is a mistake. In some cases, glslang is
> adding some extra XfbBuffer without XfbOffset around, and mentioning
> that technically that is not a bug (see issue#1526)
>
> And for the case of Vulkan, as the same glslang issue mentions, it is
> not clear if that should be a mistake or not. But even if it is a
> mistake, it is not really needed to be checked on the driver, and we
> can let the validation layers to check that.
> ---
>  src/compiler/nir/nir_gather_xfb_info.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/src/compiler/nir/nir_gather_xfb_info.c
> b/src/compiler/nir/nir_gather_xfb_info.c
> index e282bba0081..f5d831c6567 100644
> --- a/src/compiler/nir/nir_gather_xfb_info.c
> +++ b/src/compiler/nir/nir_gather_xfb_info.c
> @@ -109,9 +109,16 @@ nir_gather_xfb_info(const nir_shader *shader, void
> *mem_ctx)
>     nir_foreach_variable(var, &shader->outputs) {
>        if (var->data.explicit_xfb_buffer ||
>            var->data.explicit_xfb_stride) {
> -         assert(var->data.explicit_xfb_buffer &&
> -                var->data.explicit_xfb_stride &&
> -                var->data.explicit_offset);
> +
> +         /* OpenGL points that both are needed to capture the output, but
> +          * doesn't direcly imply that it is a mistake having one but not
> the
> +          * other.
> +          */
> +         if (!var->data.explicit_xfb_buffer ||
> +             !var->data.explicit_offset) {
>

Why not just change the check above to "var->data.explicit_xfb_buffer &&
var->data.explicit_offset" and not bother with two checks?


> +            continue;
> +         }
> +
>           num_outputs += glsl_count_attribute_slots(var->type, false);
>        }
>     }
> @@ -124,6 +131,16 @@ nir_gather_xfb_info(const nir_shader *shader, void
> *mem_ctx)
>     nir_foreach_variable(var, &shader->outputs) {
>        if (var->data.explicit_xfb_buffer ||
>            var->data.explicit_xfb_stride) {
> +
> +         /* OpenGL points that both are needed to capture the output, but
> +          * doesn't direcly imply that it is a mistake having one but not
> the
> +          * other.
> +          */
> +         if (!var->data.explicit_xfb_buffer ||
> +             !var->data.explicit_offset) {
>

Same here.


> +            continue;
> +         }
> +
>           unsigned location = var->data.location;
>           unsigned offset = var->data.offset;
>           add_var_xfb_outputs(xfb, var, &location, &offset, var->type);
> --
> 2.14.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181108/0a5b5718/attachment.html>