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

[Alejandro Piñeiro]

On 08/11/18 22:42, Jason Ekstrand wrote:
> On Thu, Nov 8, 2018 at 7:22 AM Alejandro Piñeiro <apinheiro at igalia.com
> <mailto: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?

True. Change done locally.

>  
>
>     +            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/20181109/ef94fc3d/attachment.html>