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

[Lionel Landwerlin]

Acked-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>

I can see that it fixes the tests and it makes sense, but I'm failing to 
see how gl_attrib_wa_flags ends up being set from anv :/

Thanks a lot!

On 21/07/17 09:26, 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;
>