On Fri, May 5, 2017 at 12:38 AM, Alejandro Piñeiro <apinhe...@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
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev