From 10dc4ac4c5d6dbe3df1f2b75229804e7aa5f86f1 Mon Sep 17 00:00:00 2001 From: Eric Anholt Date: Fri, 20 Dec 2019 09:02:07 -0800 Subject: [PATCH] mesa: Make atomic lowering put atomics above SSBOs. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Gallium arbitrarily (it seems) put atomics below SSBOs, resulting in a bunch of extra index management, and surprising shader code when you would see your SSBOs up at index 16. It makes a lot more sense to see atomics converted to SSBOs appear as magic high numbers. Reviewed-by: Marek Olšák Part-of: --- src/compiler/nir/nir_lower_atomics_to_ssbo.c | 69 +++++-------------- src/mesa/drivers/dri/i965/brw_link.cpp | 2 +- .../drivers/dri/i965/brw_wm_surface_state.c | 4 +- src/mesa/state_tracker/st_atom_atomicbuf.c | 22 +++--- src/mesa/state_tracker/st_atom_storagebuf.c | 7 +- src/mesa/state_tracker/st_draw_feedback.c | 4 +- src/mesa/state_tracker/st_glsl_to_nir.cpp | 2 +- src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 24 +++---- 8 files changed, 48 insertions(+), 86 deletions(-) diff --git a/src/compiler/nir/nir_lower_atomics_to_ssbo.c b/src/compiler/nir/nir_lower_atomics_to_ssbo.c index 7ff0123b7bb..df6f959c4b5 100644 --- a/src/compiler/nir/nir_lower_atomics_to_ssbo.c +++ b/src/compiler/nir/nir_lower_atomics_to_ssbo.c @@ -32,17 +32,13 @@ #endif /* - * Remap atomic counters to SSBOs. Atomic counters get remapped to - * SSBO binding points [0..ssbo_offset) and the original SSBOs are - * remapped to [ssbo_offset..n) (mostly to align with what mesa/st - * does. + * Remap atomic counters to SSBOs, starting from the passed in ssbo_offset. */ static bool lower_instr(nir_intrinsic_instr *instr, unsigned ssbo_offset, nir_builder *b) { nir_intrinsic_op op; - int idx_src; b->cursor = nir_before_instr(&instr->instr); @@ -54,32 +50,6 @@ lower_instr(nir_intrinsic_instr *instr, unsigned ssbo_offset, nir_builder *b) instr->intrinsic = nir_intrinsic_memory_barrier_buffer; return true; - case nir_intrinsic_ssbo_atomic_add: - case nir_intrinsic_ssbo_atomic_imin: - case nir_intrinsic_ssbo_atomic_umin: - case nir_intrinsic_ssbo_atomic_imax: - case nir_intrinsic_ssbo_atomic_umax: - case nir_intrinsic_ssbo_atomic_and: - case nir_intrinsic_ssbo_atomic_or: - case nir_intrinsic_ssbo_atomic_xor: - case nir_intrinsic_ssbo_atomic_exchange: - case nir_intrinsic_ssbo_atomic_comp_swap: - case nir_intrinsic_ssbo_atomic_fadd: - case nir_intrinsic_ssbo_atomic_fmin: - case nir_intrinsic_ssbo_atomic_fmax: - case nir_intrinsic_ssbo_atomic_fcomp_swap: - case nir_intrinsic_store_ssbo: - case nir_intrinsic_load_ssbo: - case nir_intrinsic_get_buffer_size: - /* easy case, keep same opcode and just remap SSBO buffer index: */ - op = instr->intrinsic; - idx_src = (op == nir_intrinsic_store_ssbo) ? 1 : 0; - nir_ssa_def *old_idx = nir_ssa_for_src(b, instr->src[idx_src], 1); - nir_ssa_def *new_idx = nir_iadd(b, old_idx, nir_imm_int(b, ssbo_offset)); - nir_instr_rewrite_src(&instr->instr, - &instr->src[idx_src], - nir_src_for_ssa(new_idx)); - return true; case nir_intrinsic_atomic_counter_inc: case nir_intrinsic_atomic_counter_add: case nir_intrinsic_atomic_counter_pre_dec: @@ -115,7 +85,7 @@ lower_instr(nir_intrinsic_instr *instr, unsigned ssbo_offset, nir_builder *b) return false; } - nir_ssa_def *buffer = nir_imm_int(b, nir_intrinsic_base(instr)); + nir_ssa_def *buffer = nir_imm_int(b, ssbo_offset + nir_intrinsic_base(instr)); nir_ssa_def *temp = NULL; nir_intrinsic_instr *new_instr = nir_intrinsic_instr_create(ralloc_parent(instr), op); @@ -231,7 +201,21 @@ nir_lower_atomics_to_ssbo(nir_shader *shader, unsigned ssbo_offset) snprintf(name, sizeof(name), "counter%d", var->data.binding); ssbo = nir_variable_create(shader, nir_var_mem_ssbo, type, name); - ssbo->data.binding = var->data.binding; + ssbo->data.binding = ssbo_offset + var->data.binding; + + /* We can't use num_abos, because it only represents the number of + * active atomic counters, and currently unlike SSBO's they aren't + * compacted so num_abos actually isn't a bound on the index passed + * to nir_intrinsic_atomic_counter_*. e.g. if we have a single atomic + * counter declared like: + * + * layout(binding=1) atomic_uint counter0; + * + * then when we lower accesses to it the atomic_counter_* intrinsics + * will have 1 as the index but num_abos will still be 1. + */ + shader->info.num_ssbos = MAX2(shader->info.num_ssbos, + ssbo->data.binding + 1); struct glsl_struct_field field = { .type = type, @@ -247,25 +231,6 @@ nir_lower_atomics_to_ssbo(nir_shader *shader, unsigned ssbo_offset) } } - /* Make sure that shader->info.num_ssbos still reflects the maximum SSBO - * index that can be used in the shader. - */ - if (shader->info.num_ssbos > 0) { - shader->info.num_ssbos += ssbo_offset; - } else { - /* We can't use num_abos, because it only represents the number of - * active atomic counters, and currently unlike SSBO's they aren't - * compacted so num_abos actually isn't a bound on the index passed - * to nir_intrinsic_atomic_counter_*. e.g. if we have a single atomic - * counter declared like: - * - * layout(binding=1) atomic_uint counter0; - * - * then when we lower accesses to it the atomic_counter_* intrinsics - * will have 1 as the index but num_abos will still be 1. - * */ - shader->info.num_ssbos = util_last_bit(replaced); - } shader->info.num_abos = 0; } diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp b/src/mesa/drivers/dri/i965/brw_link.cpp index 2a844575c3d..c439fcbc930 100644 --- a/src/mesa/drivers/dri/i965/brw_link.cpp +++ b/src/mesa/drivers/dri/i965/brw_link.cpp @@ -333,7 +333,7 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg) NIR_PASS_V(prog->nir, gl_nir_lower_atomics, shProg, false); NIR_PASS_V(prog->nir, nir_lower_atomics_to_ssbo, - prog->nir->info.num_abos); + prog->nir->info.num_ssbos); nir_sweep(prog->nir); diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c index f1defb3f148..bb2d8043f18 100644 --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c @@ -1374,9 +1374,9 @@ brw_upload_ubo_surfaces(struct brw_context *brw, struct gl_program *prog, ISL_FORMAT_R32G32B32A32_FLOAT, 0); } - uint32_t *abo_surf_offsets = + uint32_t *ssbo_surf_offsets = &stage_state->surf_offset[prog_data->binding_table.ssbo_start]; - uint32_t *ssbo_surf_offsets = abo_surf_offsets + prog->info.num_abos; + uint32_t *abo_surf_offsets = ssbo_surf_offsets + prog->info.num_ssbos; for (int i = 0; i < prog->info.num_abos; i++) { struct gl_buffer_binding *binding = diff --git a/src/mesa/state_tracker/st_atom_atomicbuf.c b/src/mesa/state_tracker/st_atom_atomicbuf.c index 5a8ff0f05f2..dad2b65b4c8 100644 --- a/src/mesa/state_tracker/st_atom_atomicbuf.c +++ b/src/mesa/state_tracker/st_atom_atomicbuf.c @@ -66,13 +66,19 @@ st_binding_to_sb(struct gl_buffer_binding *binding, static void st_bind_atomics(struct st_context *st, struct gl_program *prog, - enum pipe_shader_type shader_type) + gl_shader_stage stage) { unsigned i; + enum pipe_shader_type shader_type = pipe_shader_type_from_mesa(stage); if (!prog || !st->pipe->set_shader_buffers || st->has_hw_atomics) return; + /* For !has_hw_atomics, the atomic counters have been rewritten to be above + * the SSBO range. + */ + unsigned buffer_base = st->ctx->Const.Program[stage].MaxShaderStorageBlocks; + for (i = 0; i < prog->sh.data->NumAtomicBuffers; i++) { struct gl_active_atomic_buffer *atomic = &prog->sh.data->AtomicBuffers[i]; @@ -81,7 +87,7 @@ st_bind_atomics(struct st_context *st, struct gl_program *prog, st_binding_to_sb(&st->ctx->AtomicBufferBindings[atomic->Binding], &sb); st->pipe->set_shader_buffers(st->pipe, shader_type, - atomic->Binding, 1, &sb, 0x1); + buffer_base + atomic->Binding, 1, &sb, 0x1); } } @@ -91,7 +97,7 @@ st_bind_vs_atomics(struct st_context *st) struct gl_program *prog = st->ctx->_Shader->CurrentProgram[MESA_SHADER_VERTEX]; - st_bind_atomics(st, prog, PIPE_SHADER_VERTEX); + st_bind_atomics(st, prog, MESA_SHADER_VERTEX); } void @@ -100,7 +106,7 @@ st_bind_fs_atomics(struct st_context *st) struct gl_program *prog = st->ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT]; - st_bind_atomics(st, prog, PIPE_SHADER_FRAGMENT); + st_bind_atomics(st, prog, MESA_SHADER_FRAGMENT); } void @@ -109,7 +115,7 @@ st_bind_gs_atomics(struct st_context *st) struct gl_program *prog = st->ctx->_Shader->CurrentProgram[MESA_SHADER_GEOMETRY]; - st_bind_atomics(st, prog, PIPE_SHADER_GEOMETRY); + st_bind_atomics(st, prog, MESA_SHADER_GEOMETRY); } void @@ -118,7 +124,7 @@ st_bind_tcs_atomics(struct st_context *st) struct gl_program *prog = st->ctx->_Shader->CurrentProgram[MESA_SHADER_TESS_CTRL]; - st_bind_atomics(st, prog, PIPE_SHADER_TESS_CTRL); + st_bind_atomics(st, prog, MESA_SHADER_TESS_CTRL); } void @@ -127,7 +133,7 @@ st_bind_tes_atomics(struct st_context *st) struct gl_program *prog = st->ctx->_Shader->CurrentProgram[MESA_SHADER_TESS_EVAL]; - st_bind_atomics(st, prog, PIPE_SHADER_TESS_EVAL); + st_bind_atomics(st, prog, MESA_SHADER_TESS_EVAL); } void @@ -140,7 +146,7 @@ st_bind_cs_atomics(struct st_context *st) struct gl_program *prog = st->ctx->_Shader->CurrentProgram[MESA_SHADER_COMPUTE]; - st_bind_atomics(st, prog, PIPE_SHADER_COMPUTE); + st_bind_atomics(st, prog, MESA_SHADER_COMPUTE); } void diff --git a/src/mesa/state_tracker/st_atom_storagebuf.c b/src/mesa/state_tracker/st_atom_storagebuf.c index 5ec3175f2c9..5ffafaa611b 100644 --- a/src/mesa/state_tracker/st_atom_storagebuf.c +++ b/src/mesa/state_tracker/st_atom_storagebuf.c @@ -47,14 +47,11 @@ st_bind_ssbos(struct st_context *st, struct gl_program *prog, unsigned i; struct pipe_shader_buffer buffers[MAX_SHADER_STORAGE_BUFFERS]; struct gl_program_constants *c; - int buffer_base; if (!prog || !st->pipe->set_shader_buffers) return; c = &st->ctx->Const.Program[prog->info.stage]; - buffer_base = st->has_hw_atomics ? 0 : c->MaxAtomicBuffers; - for (i = 0; i < prog->info.num_ssbos; i++) { struct gl_buffer_binding *binding; struct st_buffer_object *st_obj; @@ -81,14 +78,14 @@ st_bind_ssbos(struct st_context *st, struct gl_program *prog, sb->buffer_size = 0; } } - st->pipe->set_shader_buffers(st->pipe, shader_type, buffer_base, + st->pipe->set_shader_buffers(st->pipe, shader_type, 0, prog->info.num_ssbos, buffers, prog->sh.ShaderStorageBlocksWriteAccess); /* clear out any stale shader buffers */ if (prog->info.num_ssbos < c->MaxShaderStorageBlocks) st->pipe->set_shader_buffers( st->pipe, shader_type, - buffer_base + prog->info.num_ssbos, + prog->info.num_ssbos, c->MaxShaderStorageBlocks - prog->info.num_ssbos, NULL, 0); } diff --git a/src/mesa/state_tracker/st_draw_feedback.c b/src/mesa/state_tracker/st_draw_feedback.c index b31745ffae5..0b70a08a870 100644 --- a/src/mesa/state_tracker/st_draw_feedback.c +++ b/src/mesa/state_tracker/st_draw_feedback.c @@ -259,8 +259,6 @@ st_feedback_draw_vbo(struct gl_context *ctx, /* shader buffers */ /* TODO: atomic counter buffers */ struct pipe_transfer *ssbo_transfer[PIPE_MAX_SHADER_BUFFERS] = {0}; - unsigned ssbo_first_slot = st->has_hw_atomics ? 0 : - st->ctx->Const.Program[MESA_SHADER_VERTEX].MaxAtomicBuffers; for (unsigned i = 0; i < prog->info.num_ssbos; i++) { struct gl_buffer_binding *binding = @@ -285,7 +283,7 @@ st_feedback_draw_vbo(struct gl_context *ctx, PIPE_TRANSFER_READ, &ssbo_transfer[i]); draw_set_mapped_shader_buffer(draw, PIPE_SHADER_VERTEX, - ssbo_first_slot + i, ptr, size); + i, ptr, size); } /* samplers */ diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp b/src/mesa/state_tracker/st_glsl_to_nir.cpp index 9c6031f4a19..d19398bd4b9 100644 --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp @@ -505,7 +505,7 @@ st_glsl_to_nir_post_opts(struct st_context *st, struct gl_program *prog, if (!st->has_hw_atomics) NIR_PASS_V(nir, nir_lower_atomics_to_ssbo, - st->ctx->Const.Program[nir->info.stage].MaxAtomicBuffers); + st->ctx->Const.Program[nir->info.stage].MaxShaderStorageBlocks); st_finalize_nir_before_variants(nir); diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp index 0416a0b02a1..aec59e75e71 100644 --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp @@ -2237,11 +2237,9 @@ glsl_to_tgsi_visitor::visit_expression(ir_expression* ir, st_src_reg *op) case ir_unop_get_buffer_size: { ir_constant *const_offset = ir->operands[0]->as_constant(); - int buf_base = ctx->st->has_hw_atomics - ? 0 : ctx->Const.Program[shader->Stage].MaxAtomicBuffers; st_src_reg buffer( PROGRAM_BUFFER, - buf_base + (const_offset ? const_offset->value.u[0] : 0), + const_offset ? const_offset->value.u[0] : 0, GLSL_TYPE_UINT); if (!const_offset) { buffer.reladdr = ralloc(mem_ctx, st_src_reg); @@ -3449,7 +3447,9 @@ glsl_to_tgsi_visitor::visit_atomic_counter_intrinsic(ir_call *ir) resource = buffer; } else { - st_src_reg buffer(PROGRAM_BUFFER, location->data.binding, + st_src_reg buffer(PROGRAM_BUFFER, + ctx->Const.Program[shader->Stage].MaxShaderStorageBlocks + + location->data.binding, GLSL_TYPE_ATOMIC_UINT); if (offset.file != PROGRAM_UNDEFINED) { @@ -3537,11 +3537,9 @@ glsl_to_tgsi_visitor::visit_ssbo_intrinsic(ir_call *ir) ir_rvalue *offset = ((ir_instruction *)param)->as_rvalue(); ir_constant *const_block = block->as_constant(); - int buf_base = st_context(ctx)->has_hw_atomics - ? 0 : ctx->Const.Program[shader->Stage].MaxAtomicBuffers; st_src_reg buffer( PROGRAM_BUFFER, - buf_base + (const_block ? const_block->value.u[0] : 0), + const_block ? const_block->value.u[0] : 0, GLSL_TYPE_UINT); if (!const_block) { @@ -7053,8 +7051,10 @@ st_translate_program( if (!st_context(ctx)->has_hw_atomics) { for (i = 0; i < prog->info.num_abos; i++) { - unsigned index = prog->sh.AtomicBuffers[i]->Binding; - assert(index < frag_const->MaxAtomicBuffers); + unsigned index = (frag_const->MaxShaderStorageBlocks + + prog->sh.AtomicBuffers[i]->Binding); + assert(prog->sh.AtomicBuffers[i]->Binding < + frag_const->MaxAtomicBuffers); t->buffers[index] = ureg_DECL_buffer(ureg, index, true); } } else { @@ -7069,11 +7069,7 @@ st_translate_program( assert(prog->info.num_ssbos <= frag_const->MaxShaderStorageBlocks); for (i = 0; i < prog->info.num_ssbos; i++) { - unsigned index = i; - if (!st_context(ctx)->has_hw_atomics) - index += frag_const->MaxAtomicBuffers; - - t->buffers[index] = ureg_DECL_buffer(ureg, index, false); + t->buffers[i] = ureg_DECL_buffer(ureg, i, false); } } -- 2.30.2