From 25efd787cfd842c0b0b900f35399e44a2e01ea39 Mon Sep 17 00:00:00 2001 From: Jason Ekstrand Date: Thu, 30 Aug 2018 15:02:25 -0500 Subject: [PATCH] compiler: Move double_inputs to gl_program::DualSlotInputs MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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. Tested-by: Alejandro Piñeiro (ARB_gl_spirv tests) Reviewed-by: Alejandro Piñeiro Reviewed-by: Timothy Arceri --- 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 | 49 ++++++++++++++----- src/compiler/nir/nir.h | 6 ++- src/compiler/nir/nir_gather_info.c | 6 --- src/compiler/shader_info.h | 3 -- src/mesa/drivers/dri/i965/brw_draw_upload.c | 21 ++++---- 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, 83 insertions(+), 65 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..0d8a554bd20 100644 --- a/src/compiler/nir/nir.c +++ b/src/compiler/nir/nir.c @@ -1854,23 +1854,48 @@ 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); + + nir_foreach_variable(var, &shader->inputs) { + var->data.location += + _mesa_bitcount_64(dual_slot & BITFIELD64_MASK(var->data.location)); } +} - /* 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; +/* Returns an attribute mask that has been re-compacted using the given + * dual_slot mask. + */ +uint64_t +nir_get_single_slot_attribs_mask(uint64_t attribs, uint64_t dual_slot) +{ + while (dual_slot) { + unsigned loc = u_bit_scan64(&dual_slot); + /* mask of all bits up to and including loc */ + uint64_t mask = BITFIELD64_MASK(loc + 1); + attribs = (attribs & mask) | ((attribs & ~mask) >> 1); + } + return attribs; } diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h index 169fa1fa20d..b9393702097 100644 --- a/src/compiler/nir/nir.h +++ b/src/compiler/nir/nir.h @@ -3039,8 +3039,10 @@ 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); +uint64_t nir_get_single_slot_attribs_mask(uint64_t attribs, 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..dc3022bc417 100644 --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c @@ -29,6 +29,7 @@ #include "main/enums.h" #include "main/macros.h" #include "main/glformats.h" +#include "nir.h" #include "brw_draw.h" #include "brw_defines.h" @@ -454,10 +455,14 @@ 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); - GLbitfield64 vs_inputs = vs_prog_data->inputs_read; + GLbitfield64 vs_inputs = + nir_get_single_slot_attribs_mask(vs_prog_data->inputs_read, + vp->DualSlotInputs); const unsigned char *ptr = NULL; GLuint interleaved = 0; unsigned int min_index = brw->vb.min_index + brw->basevertex; @@ -486,16 +491,12 @@ 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); + const unsigned index = ffsll(vs_inputs) - 1; + assert(index < 64); + 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; + vs_inputs &= ~BITFIELD64_BIT(index); 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 68573f628db..ffaaeff77a5 100644 --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp @@ -7129,7 +7129,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++; -- 2.30.2