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

[Iago Toral Quiroga]

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