On Thursday, January 14, 2016 10:23:45 AM PST Matt Turner wrote: > On Wed, Jan 13, 2016 at 8:33 PM, Kenneth Graunke <[email protected]> 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 <[email protected]> > > --- > > > > 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.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
