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

[Iago Toral]

On Wed, 2017-08-09 at 16:18 +0100, Lionel Landwerlin wrote:
> 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 :/

Sorry, I should've explained that. It is not really anv but the shared
compiler infrastructure that does this. From anv, the call chain is
this:

anv_pipeline_compile_vs -> brw_nir_lower_vs_inputs ->
brw_nir_apply_attribute_workarounds

brw_nir_lower_vs_inputs is the one that reads gl_attrib_wa_flags from
the brw_vs_prog_key state, which is eventually used used by
brw_nir_apply_attribute_workarounds (where the out-of-bounds accessed
happen without this patch).

Iago

> 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;
> >   
> 
> 
>