[Mesa-dev] [PATCH 3/3] intel/compiler: properly size attribute wa_flags array for Vulkan

[Iago Toral]

This patch is still missing a review. Any takers?

Iago

On Fri, 2017-07-21 at 10:26 +0200, Iago Toral Quiroga wrote:
> Mesa will map user defined vertex input attributes to slots
> starting at VERT_ATTRIB_GENERIC0 which gives us room for only 16
> slots (up to GL_VERT_ATTRIB_MAX). This sufficient for GL, where
> we expose exactly 16 vertex attributes for user defined inputs, but
> in Vulkan we can expose up to 28 (which are also mapped from
> VERT_ATTRIB_GENERIC0 onwards) so we need to account for this when
> we scope the size of the array of attribute workaround flags
> that is used during the brw_vertex_workarounds NIR pass. This
> prevents out-of-bounds accesses in that array for NIR shaders
> that use more than 16 vertex input attributes.
> 
> Fixes:
> dEQP-VK.pipeline.vertex_input.max_attributes.*
> ---
> 
> I considered other options for this too:
> 
> *  Increase the value of GL_VERT_ATTRIB_MAX, but that would affect
> other drivers, including GL drivers that do not need to increase
> this value. If we do this we should at least check that increasing
> the value doesn't have unexpected consequences for drivers that
> rely in GL_VERT_ATTRIB_MAX not being larger than what it currently
> is.
> 
> * We could remap vulkan vertex attrib slots to start at 0 at some
> point in the process... but I think that could be a source of
> confusion, since from that point forward we shouldn't be using
> shader enums to identify particular slots and there would also be
> a mismatch between input bits in vs_prog_data->inputs_read
> and actual slot positions which looks like an undesirable situation.
> 
> * Since the brw_vertex_workarounds pass seems rather GL-specific at
> the moment, we could let our compiler infrastructure know if we
> are compiling a shader for Vulkan or GL APIs and use that info
> to not run the pass for Vulkan shaders, however, there is a chance
> that we need this pass in Vulkan in the future too (maybe with
> Vulkan specific lowerings). There is actually a comment in
> anv_pipipeline.c, function populate_vs_prog_key() suggesting this
> possibility, so this solution would be invalid if that ever happened.
> 
> Since all solutions have some kind of con I decided to go with the
> less intrusive one for now.
> 
>  src/intel/compiler/brw_compiler.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/src/intel/compiler/brw_compiler.h
> b/src/intel/compiler/brw_compiler.h
> index bebd244736..41c0707003 100644
> --- a/src/intel/compiler/brw_compiler.h
> +++ b/src/intel/compiler/brw_compiler.h
> @@ -188,6 +188,15 @@ struct brw_sampler_prog_key_data {
>  #define BRW_ATTRIB_WA_SIGN          32  /* interpret as signed in
> shader */
>  #define BRW_ATTRIB_WA_SCALE         64  /* interpret as scaled in
> shader */
>  
> +/**
> + * OpenGL attribute slots fall in [0, VERT_ATTRIB_MAX - 1] with the
> range
> + * [VERT_ATTRIB_GENERIC0, VERT_ATTRIB_MAX - 1] reserved for up to 16
> user
> + * input vertex attributes. In Vulkan, we expose up to 28 user
> vertex input
> + * attributes that are mapped to slots also starting at
> VERT_ATTRIB_GENERIC0.
> + */
> +#define MAX_GL_VERT_ATTRIB     VERT_ATTRIB_MAX
> +#define MAX_VK_VERT_ATTRIB     (VERT_ATTRIB_GENERIC0 + 28)
> +
>  /** The program key for Vertex Shaders. */
>  struct brw_vs_prog_key {
>     unsigned program_string_id;
> @@ -196,8 +205,15 @@ struct brw_vs_prog_key {
>      * Per-attribute workaround flags
>      *
>      * For each attribute, a combination of BRW_ATTRIB_WA_*.
> +    *
> +    * For OpenGL, where we expose a maximum of 16 user input
> atttributes
> +    * we only need up to VERT_ATTRIB_MAX slots, however, in Vulkan
> +    * slots preceding VERT_ATTRIB_GENERIC0 are unused and we can
> +    * expose up to 28 user input vertex attributes that are mapped
> to slots
> +    * starting at VERT_ATTRIB_GENERIC0, so this array need to be
> large
> +    * enough to hold this many slots.
>      */
> -   uint8_t gl_attrib_wa_flags[VERT_ATTRIB_MAX];
> +   uint8_t gl_attrib_wa_flags[MAX2(MAX_GL_VERT_ATTRIB,
> MAX_VK_VERT_ATTRIB)];
>  
>     bool copy_edgeflag:1;
>