As right now piglit only have one ARB_gl_spirv test with double inputs, and Im not really sure if intel CI already runs piglit with spirv-tools available, I tested this patch with my local piglit, that includes all the va64 tests converted to SPIR-V (that as some already know/complained about, that means ~20k tests). No regressions. So, on the ARB_gl_spirv part:
Tested-by: Alejandro Piñeiro <[email protected]> On 31/08/18 01:48, Jason Ekstrand wrote: > Previously, we had two field in shader_info: double_inputs_read and > double_inputs. Presumably, the one was for all double inputs that are > read and the other is all that exist. However, because nir_gather_info > regenerates these two values, there is a possibility, if a variable gets > deleted, that the value of double_inputs could change over time. This > is a problem because double_inputs is used to remap the input locations > to a two-slot-per-dvec3/4 scheme for i965. If that mapping were to > change between glsl_to_nir and back-end state setup, we would fall over > when trying to map the NIR outputs back onto the GL location space. > > This commit changes the way slot re-mapping works. Instead of the > double_inputs field in shader_info, it adds a DualSlotInputs bitfield to > gl_program. By having it in gl_program, we more easily guarantee that > NIR passes won't touch it after it's been set. It also makes more sense > to put it in a GL data structure since it's really a mapping from GL > slots to back-end and/or NIR slots and not really a NIR shader thing. > > This shouldn't affect gallium drivers or radv but I have yet to actually > test it on any of them. > > Cc: Kenneth Graunke <[email protected]> > Cc: Timothy Arceri <[email protected]> > > --- > src/compiler/glsl/glsl_to_nir.cpp | 16 +++------ > src/compiler/glsl/ir_set_program_inouts.cpp | 2 +- > src/compiler/glsl/serialize.cpp | 2 ++ > src/compiler/nir/nir.c | 36 ++++++++++++------- > src/compiler/nir/nir.h | 5 +-- > src/compiler/nir/nir_gather_info.c | 6 ---- > src/compiler/shader_info.h | 3 -- > src/mesa/drivers/dri/i965/brw_draw_upload.c | 19 +++++----- > src/mesa/drivers/dri/i965/genX_state_upload.c | 1 + > src/mesa/main/glspirv.c | 20 +++-------- > src/mesa/main/mtypes.h | 15 ++++++++ > src/mesa/state_tracker/st_glsl_to_nir.cpp | 2 +- > src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 2 +- > src/mesa/state_tracker/st_program.c | 3 +- > 14 files changed, 66 insertions(+), 66 deletions(-) > > diff --git a/src/compiler/glsl/glsl_to_nir.cpp > b/src/compiler/glsl/glsl_to_nir.cpp > index f38d280d406..d22f4a58dd4 100644 > --- a/src/compiler/glsl/glsl_to_nir.cpp > +++ b/src/compiler/glsl/glsl_to_nir.cpp > @@ -149,8 +149,11 @@ glsl_to_nir(const struct gl_shader_program *shader_prog, > * two locations. For instance, if we have in the IR code a dvec3 attr0 in > * location 0 and vec4 attr1 in location 1, in NIR attr0 will use > * locations/slots 0 and 1, and attr1 will use location/slot 2 */ > - if (shader->info.stage == MESA_SHADER_VERTEX) > - nir_remap_attributes(shader, options); > + if (shader->info.stage == MESA_SHADER_VERTEX) { > + sh->Program->DualSlotInputs = nir_get_dual_slot_attributes(shader); > + if (options->vs_inputs_dual_locations) > + nir_remap_dual_slot_attributes(shader, sh->Program->DualSlotInputs); > + } > > shader->info.name = ralloc_asprintf(shader, "GLSL%d", shader_prog->Name); > if (shader_prog->Label) > @@ -344,15 +347,6 @@ nir_visitor::visit(ir_variable *ir) > var->data.compact = ir->type->without_array()->is_scalar(); > } > } > - > - /* Mark all the locations that require two slots */ > - if (shader->info.stage == MESA_SHADER_VERTEX && > - glsl_type_is_dual_slot(glsl_without_array(var->type))) { > - for (unsigned i = 0; i < glsl_count_attribute_slots(var->type, > true); i++) { > - uint64_t bitfield = BITFIELD64_BIT(var->data.location + i); > - shader->info.vs.double_inputs |= bitfield; > - } > - } > break; > > case ir_var_shader_out: > diff --git a/src/compiler/glsl/ir_set_program_inouts.cpp > b/src/compiler/glsl/ir_set_program_inouts.cpp > index ba1e44167c3..a3cb19479b8 100644 > --- a/src/compiler/glsl/ir_set_program_inouts.cpp > +++ b/src/compiler/glsl/ir_set_program_inouts.cpp > @@ -118,7 +118,7 @@ mark(struct gl_program *prog, ir_variable *var, int > offset, int len, > /* double inputs read is only for vertex inputs */ > if (stage == MESA_SHADER_VERTEX && > var->type->without_array()->is_dual_slot()) > - prog->info.vs.double_inputs_read |= bitfield; > + prog->DualSlotInputs |= bitfield; > > if (stage == MESA_SHADER_FRAGMENT) { > prog->info.fs.uses_sample_qualifier |= var->data.sample; > diff --git a/src/compiler/glsl/serialize.cpp b/src/compiler/glsl/serialize.cpp > index 889038fb5e2..267700e7e78 100644 > --- a/src/compiler/glsl/serialize.cpp > +++ b/src/compiler/glsl/serialize.cpp > @@ -1035,6 +1035,7 @@ write_shader_metadata(struct blob *metadata, > gl_linked_shader *shader) > struct gl_program *glprog = shader->Program; > unsigned i; > > + blob_write_uint64(metadata, glprog->DualSlotInputs); > blob_write_bytes(metadata, glprog->TexturesUsed, > sizeof(glprog->TexturesUsed)); > blob_write_uint64(metadata, glprog->SamplersUsed); > @@ -1088,6 +1089,7 @@ read_shader_metadata(struct blob_reader *metadata, > { > unsigned i; > > + glprog->DualSlotInputs = blob_read_uint64(metadata); > blob_copy_bytes(metadata, (uint8_t *) glprog->TexturesUsed, > sizeof(glprog->TexturesUsed)); > glprog->SamplersUsed = blob_read_uint64(metadata); > diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c > index 7ae46845191..f450720dd79 100644 > --- a/src/compiler/nir/nir.c > +++ b/src/compiler/nir/nir.c > @@ -1854,23 +1854,33 @@ nir_system_value_from_intrinsic(nir_intrinsic_op > intrin) > } > } > > +uint64_t > +nir_get_dual_slot_attributes(nir_shader *shader) > +{ > + assert(shader->info.stage == MESA_SHADER_VERTEX); > + > + uint64_t dual_slot = 0; > + nir_foreach_variable(var, &shader->inputs) { > + if (glsl_type_is_dual_slot(glsl_without_array(var->type))) { > + unsigned slots = glsl_count_attribute_slots(var->type, true); > + dual_slot |= BITFIELD64_MASK(slots) << var->data.location; > + } > + } > + > + return dual_slot; > +} > + > /* OpenGL utility method that remaps the location attributes if they are > * doubles. Not needed for vulkan due the differences on the input location > * count for doubles on vulkan vs OpenGL > */ > void > -nir_remap_attributes(nir_shader *shader, > - const nir_shader_compiler_options *options) > -{ > - if (options->vs_inputs_dual_locations) { > - nir_foreach_variable(var, &shader->inputs) { > - var->data.location += > - _mesa_bitcount_64(shader->info.vs.double_inputs & > - BITFIELD64_MASK(var->data.location)); > - } > - } > +nir_remap_dual_slot_attributes(nir_shader *shader, uint64_t dual_slot) > +{ > + assert(shader->info.stage == MESA_SHADER_VERTEX); > > - /* Once the remap is done, reset double_inputs_read, so later it will have > - * which location/slots are doubles */ > - shader->info.vs.double_inputs = 0; > + nir_foreach_variable(var, &shader->inputs) { > + var->data.location += > + _mesa_bitcount_64(dual_slot & BITFIELD64_MASK(var->data.location)); > + } > } > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > index 169fa1fa20d..309dda673be 100644 > --- a/src/compiler/nir/nir.h > +++ b/src/compiler/nir/nir.h > @@ -3039,8 +3039,9 @@ bool nir_opt_conditional_discard(nir_shader *shader); > > void nir_sweep(nir_shader *shader); > > -void nir_remap_attributes(nir_shader *shader, > - const nir_shader_compiler_options *options); > +uint64_t nir_get_dual_slot_attributes(nir_shader *shader); > +void nir_remap_dual_slot_attributes(nir_shader *shader, > + uint64_t dual_slot); > > nir_intrinsic_op nir_intrinsic_from_system_value(gl_system_value val); > gl_system_value nir_system_value_from_intrinsic(nir_intrinsic_op intrin); > diff --git a/src/compiler/nir/nir_gather_info.c > b/src/compiler/nir/nir_gather_info.c > index 4a030cb6256..de18c9bd78e 100644 > --- a/src/compiler/nir/nir_gather_info.c > +++ b/src/compiler/nir/nir_gather_info.c > @@ -54,11 +54,6 @@ set_io_mask(nir_shader *shader, nir_variable *var, int > offset, int len, > else > shader->info.inputs_read |= bitfield; > > - /* double inputs read is only for vertex inputs */ > - if (shader->info.stage == MESA_SHADER_VERTEX && > - glsl_type_is_dual_slot(glsl_without_array(var->type))) > - shader->info.vs.double_inputs_read |= bitfield; > - > if (shader->info.stage == MESA_SHADER_FRAGMENT) { > shader->info.fs.uses_sample_qualifier |= var->data.sample; > } > @@ -417,7 +412,6 @@ nir_shader_gather_info(nir_shader *shader, > nir_function_impl *entrypoint) > shader->info.system_values_read = 0; > if (shader->info.stage == MESA_SHADER_VERTEX) { > shader->info.vs.double_inputs = 0; > - shader->info.vs.double_inputs_read = 0; > } > if (shader->info.stage == MESA_SHADER_FRAGMENT) { > shader->info.fs.uses_sample_qualifier = false; > diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h > index dab15b58894..65bc0588d67 100644 > --- a/src/compiler/shader_info.h > +++ b/src/compiler/shader_info.h > @@ -134,9 +134,6 @@ typedef struct shader_info { > struct { > /* Which inputs are doubles */ > uint64_t double_inputs; > - > - /* Which inputs are actually read and are double */ > - uint64_t double_inputs_read; > } vs; > > struct { > diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c > b/src/mesa/drivers/dri/i965/brw_draw_upload.c > index bc9b2566deb..6a29d562cb2 100644 > --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c > +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c > @@ -454,6 +454,8 @@ brw_prepare_vertices(struct brw_context *brw) > { > const struct gen_device_info *devinfo = &brw->screen->devinfo; > struct gl_context *ctx = &brw->ctx; > + /* BRW_NEW_VERTEX_PROGRAM */ > + const struct gl_program *vp = brw->programs[MESA_SHADER_VERTEX]; > /* BRW_NEW_VS_PROG_DATA */ > const struct brw_vs_prog_data *vs_prog_data = > brw_vs_prog_data(brw->vs.base.prog_data); > @@ -485,17 +487,14 @@ brw_prepare_vertices(struct brw_context *brw) > > /* Accumulate the list of enabled arrays. */ > brw->vb.nr_enabled = 0; > - while (vs_inputs) { > - GLuint first = ffsll(vs_inputs) - 1; > - assert (first < 64); > - GLuint index = > - first - > DIV_ROUND_UP(_mesa_bitcount_64(vs_prog_data->double_inputs_read & > - BITFIELD64_MASK(first)), 2); > + for (unsigned index = 0; index < VERT_ATTRIB_MAX; index++) { > + const unsigned location = > + index + _mesa_bitcount_64(vp->DualSlotInputs & > BITFIELD64_MASK(index)); > + if (!(vs_inputs & BITFIELD64_BIT(location))) > + continue; > + > struct brw_vertex_element *input = &brw->vb.inputs[index]; > - input->is_dual_slot = (vs_prog_data->double_inputs_read & > BITFIELD64_BIT(first)) != 0; > - vs_inputs &= ~BITFIELD64_BIT(first); > - if (input->is_dual_slot) > - vs_inputs &= ~BITFIELD64_BIT(first + 1); > + input->is_dual_slot = (vp->DualSlotInputs & BITFIELD64_BIT(index)) != > 0; > brw->vb.enabled[brw->vb.nr_enabled++] = input; > } > > diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c > b/src/mesa/drivers/dri/i965/genX_state_upload.c > index 09a42e44b08..740cb0c4d2e 100644 > --- a/src/mesa/drivers/dri/i965/genX_state_upload.c > +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c > @@ -933,6 +933,7 @@ static const struct brw_tracked_state genX(vertices) = { > .mesa = _NEW_POLYGON, > .brw = BRW_NEW_BATCH | > BRW_NEW_BLORP | > + BRW_NEW_VERTEX_PROGRAM | > BRW_NEW_VERTICES | > BRW_NEW_VS_PROG_DATA, > }, > diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c > index 1c5b7dd17f3..c53fe0bd07c 100644 > --- a/src/mesa/main/glspirv.c > +++ b/src/mesa/main/glspirv.c > @@ -182,20 +182,6 @@ _mesa_spirv_link_shaders(struct gl_context *ctx, struct > gl_shader_program *prog) > prog->last_vert_prog = prog->_LinkedShaders[last_vert_stage - > 1]->Program; > } > > -static void > -nir_compute_double_inputs(nir_shader *shader, > - const nir_shader_compiler_options *options) > -{ > - nir_foreach_variable(var, &shader->inputs) { > - if (glsl_type_is_dual_slot(glsl_without_array(var->type))) { > - for (unsigned i = 0; i < glsl_count_attribute_slots(var->type, > true); i++) { > - uint64_t bitfield = BITFIELD64_BIT(var->data.location + i); > - shader->info.vs.double_inputs |= bitfield; > - } > - } > - } > -} > - > nir_shader * > _mesa_spirv_to_nir(struct gl_context *ctx, > const struct gl_shader_program *prog, > @@ -278,8 +264,10 @@ _mesa_spirv_to_nir(struct gl_context *ctx, > NIR_PASS_V(nir, nir_split_per_member_structs); > > if (nir->info.stage == MESA_SHADER_VERTEX) { > - nir_compute_double_inputs(nir, options); > - nir_remap_attributes(nir, options); > + uint64_t dual_slot_inputs = nir_get_dual_slot_attributes(nir); > + if (options->vs_inputs_dual_locations) > + nir_remap_dual_slot_attributes(nir, dual_slot_inputs); > + linked_shader->Program->DualSlotInputs = dual_slot_inputs; > } > > return nir; > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h > index 5ff0d3227a8..9ed49b7ff24 100644 > --- a/src/mesa/main/mtypes.h > +++ b/src/mesa/main/mtypes.h > @@ -2066,6 +2066,21 @@ struct gl_program > /** Is this program written to on disk shader cache */ > bool program_written_to_cache; > > + /** A bitfield indicating which vertex shader inputs consume two slots > + * > + * This is used for mapping from single-slot input locations in the GL API > + * to dual-slot double input locations in the shader. This field is set > + * once as part of linking and never updated again to ensure the mapping > + * remains consistent. > + * > + * Note: There may be dual-slot variables in the original shader source > + * which do not appear in this bitfield due to having been eliminated by > + * the compiler prior to DualSlotInputs being calculated. There may also > + * be bits set in this bitfield which are set but which the shader never > + * reads due to compiler optimizations eliminating such variables after > + * DualSlotInputs is calculated. > + */ > + GLbitfield64 DualSlotInputs; > /** Subset of OutputsWritten outputs written with non-zero index. */ > GLbitfield64 SecondaryOutputsWritten; > /** TEXTURE_x_BIT bitmask */ > diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp > b/src/mesa/state_tracker/st_glsl_to_nir.cpp > index ae2c49960c9..0ee9bd9fef1 100644 > --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp > +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp > @@ -91,7 +91,7 @@ st_nir_assign_vs_in_locations(struct gl_program *prog, > nir_shader *nir) > if ((prog->info.inputs_read & BITFIELD64_BIT(attr)) != 0) { > input_to_index[attr] = num_inputs; > num_inputs++; > - if ((prog->info.vs.double_inputs_read & BITFIELD64_BIT(attr)) != 0) > { > + if ((prog->DualSlotInputs & BITFIELD64_BIT(attr)) != 0) { > /* add placeholder for second part of a double attribute */ > num_inputs++; > } > diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp > b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp > index 7b96947c607..a30672dbd62 100644 > --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp > +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp > @@ -7128,7 +7128,7 @@ get_mesa_program_tgsi(struct gl_context *ctx, > _mesa_copy_linked_program_data(shader_program, shader); > shrink_array_declarations(v->inputs, v->num_inputs, > &prog->info.inputs_read, > - prog->info.vs.double_inputs_read, > + prog->DualSlotInputs, > &prog->info.patch_inputs_read); > shrink_array_declarations(v->outputs, v->num_outputs, > &prog->info.outputs_written, 0ULL, > diff --git a/src/mesa/state_tracker/st_program.c > b/src/mesa/state_tracker/st_program.c > index 8117f4ff8db..af86c47b945 100644 > --- a/src/mesa/state_tracker/st_program.c > +++ b/src/mesa/state_tracker/st_program.c > @@ -406,8 +406,7 @@ st_translate_vertex_program(struct st_context *st, > stvp->input_to_index[attr] = stvp->num_inputs; > stvp->index_to_input[stvp->num_inputs] = attr; > stvp->num_inputs++; > - if ((stvp->Base.info.vs.double_inputs_read & > - BITFIELD64_BIT(attr)) != 0) { > + if ((stvp->Base.DualSlotInputs & BITFIELD64_BIT(attr)) != 0) { > /* add placeholder for second part of a double attribute */ > stvp->index_to_input[stvp->num_inputs] = > ST_DOUBLE_ATTRIB_PLACEHOLDER; > stvp->num_inputs++; _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
