[Mesa-dev] [RFC] compiler: Move double_inputs to gl_program::DualSlotInputs
Alejandro Piñeiro
apinheiro at igalia.com
Fri Aug 31 10:44:28 UTC 2018
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 <apinheiro at igalia.com>
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 <kenneth at whitecape.org>
> Cc: Timothy Arceri <tarceri at itsqueeze.com>
>
> ---
> 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++;
More information about the mesa-dev
mailing list