[Mesa-dev] [PATCH 5/7] i965: Apply VS attribute workarounds in NIR.

Kenneth Graunke kenneth at whitecape.org
Thu Jan 14 11:02:32 PST 2016


On Thursday, January 14, 2016 10:23:45 AM PST Matt Turner wrote:
> On Wed, Jan 13, 2016 at 8:33 PM, Kenneth Graunke <kenneth at whitecape.org> 
wrote:
> > This patch re-implements the pre-Haswell VS attribute workarounds.
> > Instead of emitting shader code in the vec4 backend, we now simply
> > call a NIR pass to emit the necessary code.
> > 
> > This simplifies the vec4 backend.  Beyond deleting code, it removes
> > the primary use of ATTR as a destination.  It also eliminates the
> > requirement that the vec4 VS backend express the ATTR file in terms
> > of VERT_ATTRIB_* locations, giving us a bit more flexibility.
> > 
> > This approach is a little different: rather than munging the attributes
> > at the top, we emit code to fix them up when they're accessed.  However,
> > we run the optimizer afterwards, so CSE should eliminate the redundant
> > math.  It may even be able to fuse it with other calculations based on
> > the input value.
> > 
> > shader-db does not handle non-default NOS settings, so I have no
> > statistics about this patch.
> 
> Couldn't we hack it on (or off?) to get an idea?
> 
> > Note that the scalar backend does not implement VS attribute
> > workarounds, as they are unnecessary on hardware which allows SIMD8 VS.
> > 
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> > 
> >  src/mesa/drivers/dri/i965/Makefile.sources         |   1 +
> >  src/mesa/drivers/dri/i965/brw_nir.c                |  19 ++-
> >  src/mesa/drivers/dri/i965/brw_nir.h                |   7 +-
> >  .../dri/i965/brw_nir_attribute_workarounds.c       | 178
> >  +++++++++++++++++++++ src/mesa/drivers/dri/i965/brw_shader.cpp          
> >  |   2 +-
> >  src/mesa/drivers/dri/i965/brw_vec4.cpp             |   3 +
> >  src/mesa/drivers/dri/i965/brw_vec4_tcs.cpp         |   2 +-
> >  src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp  | 109 -------------
> >  8 files changed, 204 insertions(+), 117 deletions(-)
> >  create mode 100644
> >  src/mesa/drivers/dri/i965/brw_nir_attribute_workarounds.c> 
> > diff --git a/src/mesa/drivers/dri/i965/Makefile.sources
> > b/src/mesa/drivers/dri/i965/Makefile.sources index 5aeeca5..c654f94
> > 100644
> > --- a/src/mesa/drivers/dri/i965/Makefile.sources
> > +++ b/src/mesa/drivers/dri/i965/Makefile.sources
> > @@ -42,6 +42,7 @@ i965_compiler_FILES = \
> > 
> >         brw_nir.h \
> >         brw_nir.c \
> >         brw_nir_analyze_boolean_resolves.c \
> > 
> > +       brw_nir_attribute_workarounds.c \
> > 
> >         brw_nir_opt_peephole_ffma.c \
> >         brw_nir_uniforms.cpp \
> >         brw_packed_float.c \
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_nir.c
> > b/src/mesa/drivers/dri/i965/brw_nir.c index 935529a..cdecc3d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_nir.c
> > +++ b/src/mesa/drivers/dri/i965/brw_nir.c
> > @@ -205,7 +205,9 @@ remap_patch_urb_offsets(nir_block *block, void
> > *closure)> 
> >  static void
> >  brw_nir_lower_inputs(nir_shader *nir,
> >  
> >                       const struct brw_device_info *devinfo,
> > 
> > -                     bool is_scalar)
> > +                     bool is_scalar,
> > +                     bool use_legacy_snorm_formula,
> > +                     const uint8_t *vs_attrib_wa_flags)
> > 
> >  {
> >  
> >     switch (nir->stage) {
> > 
> >     case MESA_SHADER_VERTEX:
> > @@ -225,6 +227,9 @@ brw_nir_lower_inputs(nir_shader *nir,
> > 
> >        add_const_offset_to_base(nir, nir_var_shader_in);
> > 
> > +      brw_nir_apply_attribute_workarounds(nir, use_legacy_snorm_formula,
> > +                                          vs_attrib_wa_flags);
> > +
> > 
> >        if (is_scalar) {
> >        
> >           /* Finally, translate VERT_ATTRIB_* values into the actual
> >           registers.
> >           
> >            *
> > 
> > @@ -497,12 +502,15 @@ brw_preprocess_nir(nir_shader *nir, bool is_scalar)
> > 
> >  nir_shader *
> >  brw_nir_lower_io(nir_shader *nir,
> >  
> >                   const struct brw_device_info *devinfo,
> > 
> > -                 bool is_scalar)
> > +                 bool is_scalar,
> > +                 bool use_legacy_snorm_formula,
> > +                 const uint8_t *vs_attrib_wa_flags)
> > 
> >  {
> >  
> >     bool progress; /* Written by OPT and OPT_V */
> >     (void)progress;
> > 
> > -   OPT_V(brw_nir_lower_inputs, devinfo, is_scalar);
> > +   OPT_V(brw_nir_lower_inputs, devinfo, is_scalar,
> > +         use_legacy_snorm_formula, vs_attrib_wa_flags);
> > 
> >     OPT_V(brw_nir_lower_outputs, devinfo, is_scalar);
> >     OPT_V(nir_lower_io, nir_var_all, is_scalar ? type_size_scalar :
> >     type_size_vec4);> 
> > @@ -613,9 +621,10 @@ brw_create_nir(struct brw_context *brw,
> > 
> >        OPT_V(nir_lower_atomics, shader_prog);
> >     
> >     }
> > 
> > -   if (nir->stage != MESA_SHADER_TESS_CTRL &&
> > +   if (nir->stage != MESA_SHADER_VERTEX &&
> > +       nir->stage != MESA_SHADER_TESS_CTRL &&
> > 
> >         nir->stage != MESA_SHADER_TESS_EVAL) {
> > 
> > -      nir = brw_nir_lower_io(nir, devinfo, is_scalar);
> > +      nir = brw_nir_lower_io(nir, devinfo, is_scalar, false, NULL);
> > 
> >     }
> >     
> >     return nir;
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_nir.h
> > b/src/mesa/drivers/dri/i965/brw_nir.h index 78b139b..5bfe40f 100644
> > --- a/src/mesa/drivers/dri/i965/brw_nir.h
> > +++ b/src/mesa/drivers/dri/i965/brw_nir.h
> > @@ -84,11 +84,16 @@ nir_shader *brw_create_nir(struct brw_context *brw,
> > 
> >  nir_shader *brw_preprocess_nir(nir_shader *nir, bool is_scalar);
> >  nir_shader *brw_nir_lower_io(nir_shader *nir,
> >  
> >                              const struct brw_device_info *devinfo,
> > 
> > -                            bool is_scalar);
> > +                            bool is_scalar,
> > +                            bool use_legacy_snorm_formula,
> > +                            const uint8_t *vs_attrib_wa_flags);
> > 
> >  nir_shader *brw_postprocess_nir(nir_shader *nir,
> >  
> >                                  const struct brw_device_info *devinfo,
> >                                  bool is_scalar);
> > 
> > +bool brw_nir_apply_attribute_workarounds(nir_shader *nir,
> > +                                         bool use_legacy_snorm_formula,
> > +                                         const uint8_t *attrib_wa_flags);
> > 
> >  nir_shader *brw_nir_apply_sampler_key(nir_shader *nir,
> >  
> >                                        const struct brw_device_info
> >                                        *devinfo,
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_nir_attribute_workarounds.c
> > b/src/mesa/drivers/dri/i965/brw_nir_attribute_workarounds.c new file mode
> > 100644
> > index 0000000..475fa1d
> > --- /dev/null
> > +++ b/src/mesa/drivers/dri/i965/brw_nir_attribute_workarounds.c
> > @@ -0,0 +1,178 @@
> > +/*
> > + * Copyright © 2016 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining
> > a
> > + * copy of this software and associated documentation files (the
> > "Software"), + * to deal in the Software without restriction, including
> > without limitation + * the rights to use, copy, modify, merge, publish,
> > distribute, sublicense, + * and/or sell copies of the Software, and to
> > permit persons to whom the + * Software is furnished to do so, subject to
> > the following conditions: + *
> > + * The above copyright notice and this permission notice (including the
> > next + * paragraph) shall be included in all copies or substantial
> > portions of the + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND
> > NONINFRINGEMENT.  IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT 
HOLDERS
> > BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN
> > ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN
> > CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE
> > SOFTWARE.
> > + */
> > +
> > +#include "glsl/nir/nir_builder.h"
> > +#include "brw_nir.h"
> > +#include "brw_vs.h"
> > +
> > +/**
> > + * Prior to Haswell, the hardware can't natively support GL_FIXED or
> > + * 2_10_10_10_REV vertex formats.  This pass inserts extra shader code
> > + * to produce the correct values.
> > + */
> > +
> > +struct attr_wa_state {
> > +   nir_builder builder;
> > +   bool impl_progress;
> > +   bool use_legacy_snorm_formula;
> > +   const uint8_t *wa_flags;
> > +};
> > +
> > +static bool
> > +apply_attr_wa_block(nir_block *block, void *void_state)
> > +{
> > +   struct attr_wa_state *state = void_state;
> > +   nir_builder *b = &state->builder;
> > +
> > +   nir_foreach_instr_safe(block, instr) {
> > +      if (instr->type != nir_instr_type_intrinsic)
> > +         continue;
> > +
> > +      nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);
> > +      if (intrin->intrinsic != nir_intrinsic_load_input)
> > +         continue;
> > +
> > +      uint8_t wa_flags = state->wa_flags[intrin->const_index[0]];
> > +      if (wa_flags == 0)
> > +         continue;
> > +
> > +      b->cursor = nir_after_instr(instr);
> > +
> > +      nir_ssa_def *val = &intrin->dest.ssa;
> > +
> > +      /* Do GL_FIXED rescaling for GLES2.0.  Our GL_FIXED attributes
> > +       * come in as floating point conversions of the integer values.
> > +       */
> > +      if (wa_flags & BRW_ATTRIB_WA_COMPONENT_MASK) {
> > +         nir_ssa_def *comps[4];
> > +         nir_ssa_def *factor = nir_imm_float(b, 1.0f / 65536.0f);
> > +         for (int i = 0; i < val->num_components; i++) {
> > +            if (i < (wa_flags & BRW_ATTRIB_WA_COMPONENT_MASK)) {
> > +               comps[i] = nir_fmul(b, nir_channel(b, val, i), factor);
> 
> It looks like the existing code uses wa_flags &
> BRW_ATTRIB_WA_COMPONENT_MASK to calculate a writemask and then does
> one multiply, while this will do potentially 3. Is that unavoidable?
> 

Hmm, that's true.  I could instead fmul the whole value, then use
nir_channel to pull out components from either the original or the
scaled value, then group them with a vec().  That would probably
be better (though my intuition isn't that great when it comes to
pulling apart and recombining values).

> The rest of the patch looks good.
> 
> It would be nice to figure out a way to get shader-db to give us some
> numbers.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20160114/b2db2b96/attachment-0001.sig>


More information about the mesa-dev mailing list