[Mesa-dev] [PATCH 6/9] i965/vec4: Use NIR remapping for VS attributes
Jason Ekstrand
jason at jlekstrand.net
Fri May 5 19:31:40 UTC 2017
On Fri, May 5, 2017 at 12:38 AM, Alejandro Piñeiro <apinheiro at igalia.com>
wrote:
> On 05/05/17 04:11, Jason Ekstrand wrote:
> > We have to pass inputs_read through from prog_data because we may add an
> > edge flag on old platforms.
>
> Well, the previous code was using nir->info->inputs_read. So perhaps
> this explanation should explicitly point that prog_data->inputs_read and
> nir->info->inputs_read they are not the same at that point (perhaps2,
> and why?)
>
Sure. How about:
We also change nir_lower_vs_inputs to take an explicit inputs_read bitmask
and pass in the inputs_read from prog_data instead from pulling it out of
NIR. This is because the version in prog_data may get EDGEFLAG added to it
on some old platforms.
> Another nitpick and one question below.
>
> > ---
> > src/intel/compiler/brw_nir.c | 12 +++----
> > src/intel/compiler/brw_nir.h | 2 +-
> > src/intel/compiler/brw_vec4.cpp | 54
> +++++++++++-------------------
> > src/intel/compiler/brw_vec4_nir.cpp | 49
> ++-------------------------
> > src/intel/compiler/brw_vec4_visitor.cpp | 7 ++--
> > src/intel/compiler/brw_vec4_vs_visitor.cpp | 31 ++---------------
> > 6 files changed, 33 insertions(+), 122 deletions(-)
> >
> > diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
> > index 556782e..5ca532f 100644
> > --- a/src/intel/compiler/brw_nir.c
> > +++ b/src/intel/compiler/brw_nir.c
> > @@ -230,7 +230,7 @@ remap_patch_urb_offsets(nir_block *block,
> nir_builder *b,
> >
> > void
> > brw_nir_lower_vs_inputs(nir_shader *nir,
> > - bool is_scalar,
> > + uint64_t inputs_read,
> > bool use_legacy_snorm_formula,
> > const uint8_t *vs_attrib_wa_flags)
> > {
> > @@ -253,11 +253,7 @@ brw_nir_lower_vs_inputs(nir_shader *nir,
> > brw_nir_apply_attribute_workarounds(nir, use_legacy_snorm_formula,
> > vs_attrib_wa_flags);
> >
> > - /* The last step is to remap VERT_ATTRIB_* to actual registers and
> we only
> > - * do that for scalar shaders at the moment.
> > - */
> > - if (!is_scalar)
> > - return;
> > + /* The last step is to remap VERT_ATTRIB_* to actual registers */
> >
> > const bool has_svgs =
> > nir->info->system_values_read &
> > @@ -266,7 +262,7 @@ brw_nir_lower_vs_inputs(nir_shader *nir,
> > BITFIELD64_BIT(SYSTEM_VALUE_VERTEX_ID_ZERO_BASE) |
> > BITFIELD64_BIT(SYSTEM_VALUE_INSTANCE_ID));
> >
> > - const unsigned num_inputs = _mesa_bitcount_64(nir->info->
> inputs_read);
> > + const unsigned num_inputs = _mesa_bitcount_64(inputs_read);
> >
> > nir_foreach_function(function, nir) {
> > if (!function->impl)
> > @@ -340,7 +336,7 @@ brw_nir_lower_vs_inputs(nir_shader *nir,
> > * before it and counting the bits.
> > */
> > int attr = nir_intrinsic_base(intrin);
> > - int slot = _mesa_bitcount_64(nir->info->inputs_read &
> > + int slot = _mesa_bitcount_64(inputs_read &
> > BITFIELD64_MASK(attr));
> > nir_intrinsic_set_base(intrin, slot);
> > break;
> > diff --git a/src/intel/compiler/brw_nir.h b/src/intel/compiler/brw_nir.h
> > index b96072c..3bba68d 100644
> > --- a/src/intel/compiler/brw_nir.h
> > +++ b/src/intel/compiler/brw_nir.h
> > @@ -98,7 +98,7 @@ nir_shader *brw_preprocess_nir(const struct
> brw_compiler *compiler,
> > bool brw_nir_lower_intrinsics(nir_shader *nir,
> > struct brw_stage_prog_data *prog_data);
> > void brw_nir_lower_vs_inputs(nir_shader *nir,
> > - bool is_scalar,
> > + uint64_t inputs_read,
> > bool use_legacy_snorm_formula,
> > const uint8_t *vs_attrib_wa_flags);
> > void brw_nir_lower_vue_inputs(nir_shader *nir, bool is_scalar,
> > diff --git a/src/intel/compiler/brw_vec4.cpp
> b/src/intel/compiler/brw_vec4.cpp
> > index 21f34bc..b387321 100644
> > --- a/src/intel/compiler/brw_vec4.cpp
> > +++ b/src/intel/compiler/brw_vec4.cpp
> > @@ -1740,40 +1740,23 @@ vec4_visitor::lower_attributes_to_hw_regs(const
> int *attribute_map,
> > int
> > vec4_vs_visitor::setup_attributes(int payload_reg)
> > {
> > - int nr_attributes;
> > - int attribute_map[VERT_ATTRIB_MAX + 2];
> > - memset(attribute_map, 0, sizeof(attribute_map));
> > -
> > - nr_attributes = 0;
> > - GLbitfield64 vs_inputs = vs_prog_data->inputs_read;
> > - while (vs_inputs) {
> > - GLuint first = ffsll(vs_inputs) - 1;
> > - int needed_slots =
> > - (vs_prog_data->double_inputs_read & BITFIELD64_BIT(first)) ?
> 2 : 1;
> > - for (int c = 0; c < needed_slots; c++) {
> > - attribute_map[first + c] = payload_reg + nr_attributes;
> > - nr_attributes++;
> > - vs_inputs &= ~BITFIELD64_BIT(first + c);
> > + foreach_block_and_inst(block, vec4_instruction, inst, cfg) {
> > + for (int i = 0; i < 3; i++) {
> > + if (inst->src[i].file == ATTR) {
> > + assert(inst->src[i].offset % REG_SIZE == 0);
> > + int grf = payload_reg + inst->src[i].nr +
> > + inst->src[i].offset / REG_SIZE;
> > +
> > + struct brw_reg reg = brw_vec8_grf(grf, 0);
> > + reg.swizzle = inst->src[i].swizzle;
> > + reg.type = inst->src[i].type;
> > + reg.abs = inst->src[i].abs;
> > + reg.negate = inst->src[i].negate;
> > + inst->src[i] = reg;
> > + }
> > }
> > }
> >
> > - /* VertexID is stored by the VF as the last vertex element, but we
> > - * don't represent it with a flag in inputs_read, so we call it
> > - * VERT_ATTRIB_MAX.
> > - */
> > - if (vs_prog_data->uses_vertexid || vs_prog_data->uses_instanceid ||
> > - vs_prog_data->uses_basevertex || vs_prog_data->uses_baseinstance)
> {
> > - attribute_map[VERT_ATTRIB_MAX] = payload_reg + nr_attributes;
> > - nr_attributes++;
> > - }
> > -
> > - if (vs_prog_data->uses_drawid) {
> > - attribute_map[VERT_ATTRIB_MAX + 1] = payload_reg + nr_attributes;
> > - nr_attributes++;
> > - }
> > -
> > - lower_attributes_to_hw_regs(attribute_map, false /* interleaved */);
> > -
> > return payload_reg + vs_prog_data->nr_attribute_slots;
> > }
> >
> > @@ -2771,10 +2754,6 @@ brw_compile_vs(const struct brw_compiler
> *compiler, void *log_data,
> > const bool is_scalar = compiler->scalar_stage[MESA_SHADER_VERTEX];
> > nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> > shader = brw_nir_apply_sampler_key(shader, compiler, &key->tex,
> is_scalar);
> > - brw_nir_lower_vs_inputs(shader, is_scalar,
> > - use_legacy_snorm_formula,
> key->gl_attrib_wa_flags);
> > - brw_nir_lower_vue_outputs(shader, is_scalar);
> > - shader = brw_postprocess_nir(shader, compiler, is_scalar);
> >
> > const unsigned *assembly = NULL;
> >
> > @@ -2791,6 +2770,11 @@ brw_compile_vs(const struct brw_compiler
> *compiler, void *log_data,
> > prog_data->inputs_read |= VERT_BIT_EDGEFLAG;
> > }
> >
> > + brw_nir_lower_vs_inputs(shader, prog_data->inputs_read,
> > + use_legacy_snorm_formula,
> key->gl_attrib_wa_flags);
> > + brw_nir_lower_vue_outputs(shader, is_scalar);
> > + shader = brw_postprocess_nir(shader, compiler, is_scalar);
> > +
> > unsigned nr_attribute_slots = _mesa_bitcount_64(prog_data->
> inputs_read);
> >
> > /* gl_VertexID and gl_InstanceID are system values, but arrive via an
> > diff --git a/src/intel/compiler/brw_vec4_nir.cpp
> b/src/intel/compiler/brw_vec4_nir.cpp
> > index a82d520..2a98932 100644
> > --- a/src/intel/compiler/brw_vec4_nir.cpp
> > +++ b/src/intel/compiler/brw_vec4_nir.cpp
> > @@ -50,45 +50,6 @@ vec4_visitor::emit_nir_code()
> > void
> > vec4_visitor::nir_setup_system_value_intrinsic(nir_intrinsic_instr
> *instr)
> > {
>
> Perhaps it would be good to include on the commit message that the
> remapping include the system values vs attributes.
>
Sure. How about:
The NIR pass already handles remapping system values to attributes for us
so we can delete the system value code as well.
> > - dst_reg *reg;
> > -
> > - switch (instr->intrinsic) {
> > - case nir_intrinsic_load_vertex_id:
> > - unreachable("should be lowered by lower_vertex_id().");
> > -
> > - case nir_intrinsic_load_vertex_id_zero_base:
> > - reg = &nir_system_values[SYSTEM_VALUE_VERTEX_ID_ZERO_BASE];
> > - if (reg->file == BAD_FILE)
> > - *reg = *make_reg_for_system_value(SYSTEM_VALUE_VERTEX_ID_ZERO_
> BASE);
> > - break;
> > -
> > - case nir_intrinsic_load_base_vertex:
> > - reg = &nir_system_values[SYSTEM_VALUE_BASE_VERTEX];
> > - if (reg->file == BAD_FILE)
> > - *reg = *make_reg_for_system_value(SYSTEM_VALUE_BASE_VERTEX);
> > - break;
> > -
> > - case nir_intrinsic_load_instance_id:
> > - reg = &nir_system_values[SYSTEM_VALUE_INSTANCE_ID];
> > - if (reg->file == BAD_FILE)
> > - *reg = *make_reg_for_system_value(SYSTEM_VALUE_INSTANCE_ID);
> > - break;
> > -
> > - case nir_intrinsic_load_base_instance:
> > - reg = &nir_system_values[SYSTEM_VALUE_BASE_INSTANCE];
> > - if (reg->file == BAD_FILE)
> > - *reg = *make_reg_for_system_value(SYSTEM_VALUE_BASE_INSTANCE);
> > - break;
> > -
> > - case nir_intrinsic_load_draw_id:
> > - reg = &nir_system_values[SYSTEM_VALUE_DRAW_ID];
> > - if (reg->file == BAD_FILE)
> > - *reg = *make_reg_for_system_value(SYSTEM_VALUE_DRAW_ID);
> > - break;
> > -
> > - default:
> > - break;
> > - }
> > }
> >
> > static bool
> > @@ -826,14 +787,8 @@ vec4_visitor::nir_emit_intrinsic(nir_intrinsic_instr
> *instr)
> > case nir_intrinsic_load_instance_id:
> > case nir_intrinsic_load_base_instance:
> > case nir_intrinsic_load_draw_id:
> > - case nir_intrinsic_load_invocation_id: {
> > - gl_system_value sv = nir_system_value_from_
> intrinsic(instr->intrinsic);
> > - src_reg val = src_reg(nir_system_values[sv]);
> > - assert(val.file != BAD_FILE);
> > - dest = get_nir_dest(instr->dest, val.type);
> > - emit(MOV(dest, val));
> > - break;
> > - }
> > + case nir_intrinsic_load_invocation_id:
> > + unreachable("should be lowered by brw_nir_lower_vs_inputs()");
> >
> > case nir_intrinsic_load_uniform: {
> > /* Offsets are in bytes but they should always be multiples of 4
> */
> > diff --git a/src/intel/compiler/brw_vec4_visitor.cpp
> b/src/intel/compiler/brw_vec4_visitor.cpp
> > index 262a084..0753bd6 100644
> > --- a/src/intel/compiler/brw_vec4_visitor.cpp
> > +++ b/src/intel/compiler/brw_vec4_visitor.cpp
> > @@ -1315,7 +1315,7 @@ vec4_visitor::emit_urb_slot(dst_reg reg, int
> varying)
> > if (output_reg[VARYING_SLOT_POS][0].file != BAD_FILE)
> > emit(MOV(reg, src_reg(output_reg[VARYING_SLOT_POS][0])));
> > break;
> > - case VARYING_SLOT_EDGE:
> > + case VARYING_SLOT_EDGE: {
> > /* This is present when doing unfilled polygons. We're supposed
> to copy
> > * the edge flag from the user-provided vertex array
> > * (glEdgeFlagPointer), or otherwise we'll copy from the current
> value
> > @@ -1323,9 +1323,12 @@ vec4_visitor::emit_urb_slot(dst_reg reg, int
> varying)
> > * determine which edges should be drawn as wireframe.
> > */
> > current_annotation = "edge flag";
> > - emit(MOV(reg, src_reg(dst_reg(ATTR, VERT_ATTRIB_EDGEFLAG,
> > + int edge_attr = _mesa_bitcount_64(nir->info->inputs_read &
> > + BITFIELD64_MASK(VERT_ATTRIB_
> EDGEFLAG));
> > + emit(MOV(reg, src_reg(dst_reg(ATTR, edge_attr,
> > glsl_type::float_type,
> WRITEMASK_XYZW))));
> > break;
> > + }
> > case BRW_VARYING_SLOT_PAD:
> > /* No need to write to this slot */
> > break;
> > diff --git a/src/intel/compiler/brw_vec4_vs_visitor.cpp
> b/src/intel/compiler/brw_vec4_vs_visitor.cpp
> > index 2a19788..7edf6ba 100644
> > --- a/src/intel/compiler/brw_vec4_vs_visitor.cpp
> > +++ b/src/intel/compiler/brw_vec4_vs_visitor.cpp
> > @@ -36,35 +36,8 @@ vec4_vs_visitor::emit_prolog()
> > dst_reg *
> > vec4_vs_visitor::make_reg_for_system_value(int location)
> > {
> > - /* VertexID is stored by the VF as the last vertex element, but
> > - * we don't represent it with a flag in inputs_read, so we call
> > - * it VERT_ATTRIB_MAX, which setup_attributes() picks up on.
> > - */
> > - dst_reg *reg = new(mem_ctx) dst_reg(ATTR, VERT_ATTRIB_MAX);
> > -
> > - switch (location) {
> > - case SYSTEM_VALUE_BASE_VERTEX:
> > - reg->writemask = WRITEMASK_X;
> > - break;
> > - case SYSTEM_VALUE_BASE_INSTANCE:
> > - reg->writemask = WRITEMASK_Y;
> > - break;
> > - case SYSTEM_VALUE_VERTEX_ID:
> > - case SYSTEM_VALUE_VERTEX_ID_ZERO_BASE:
> > - reg->writemask = WRITEMASK_Z;
> > - break;
> > - case SYSTEM_VALUE_INSTANCE_ID:
> > - reg->writemask = WRITEMASK_W;
> > - break;
> > - case SYSTEM_VALUE_DRAW_ID:
> > - reg = new(mem_ctx) dst_reg(ATTR, VERT_ATTRIB_MAX + 1);
> > - reg->writemask = WRITEMASK_X;
> > - break;
> > - default:
> > - unreachable("not reached");
> > - }
> > -
> > - return reg;
> > + unreachable("not reached");
> > + return NULL;
>
> So I understand that the only reason to not just remove
> make_reg_for_system_value is the custom make_reg_for_system_value at
> brw_vec4_gs_visitor (that deals with invocation ID), right?
>
I believe that's correct. That said, I should be able to fairly easily
just special-case that and delete even more stuff. I'll give it a go as a
follow-on patch.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170505/b2320dbc/attachment-0001.html>
More information about the mesa-dev
mailing list