From 2128a8a07b5d1fa91dbf799c63f1c452d942d89b Mon Sep 17 00:00:00 2001 From: Lionel Landwerlin Date: Wed, 7 Feb 2024 10:16:57 +0200 Subject: [PATCH] anv: fixup push descriptor shader analysis There are a couple mistakes here : - using a bitfield as an index to generate a bitfield... - in anv_nir_push_desc_ubo_fully_promoted(), confusing binding table access of the descriptor buffer with actual descriptors Signed-off-by: Lionel Landwerlin Fixes: ff91c5ca42 ("anv: add analysis for push descriptor uses and store it in shader cache") Reviewed-by: Ivan Briano Part-of: (cherry picked from commit cf193af7626d70062f3814e3111d66959afc523f) --- .pick_status.json | 2 +- src/intel/compiler/brw_nir.h | 26 +++++- .../vulkan/anv_nir_push_descriptor_analysis.c | 90 ++++++++++++------- src/intel/vulkan/anv_pipeline.c | 2 +- 4 files changed, 81 insertions(+), 39 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index fb616ef0b4e..9f8ec32da89 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -2714,7 +2714,7 @@ "description": "anv: fixup push descriptor shader analysis", "nominated": true, "nomination_type": 1, - "resolution": 0, + "resolution": 1, "main_sha": null, "because_sha": "ff91c5ca42bc80aa411cb3fd8f550aa6fdd16bdc", "notes": null diff --git a/src/intel/compiler/brw_nir.h b/src/intel/compiler/brw_nir.h index 119de1c6086..7d8870732ad 100644 --- a/src/intel/compiler/brw_nir.h +++ b/src/intel/compiler/brw_nir.h @@ -122,8 +122,7 @@ brw_nir_ubo_surface_index_is_pushable(nir_src src) if (intrin && intrin->intrinsic == nir_intrinsic_resource_intel) { return (nir_intrinsic_resource_access_intel(intrin) & - nir_resource_intel_pushable) && - nir_src_is_const(intrin->src[1]); + nir_resource_intel_pushable); } return nir_src_is_const(src); @@ -146,6 +145,14 @@ brw_nir_ubo_surface_index_get_push_block(nir_src src) return nir_intrinsic_resource_block_intel(intrin); } +/* This helper return the binding table index of a surface access (any + * buffer/image/etc...). It works off the source of one of the intrinsics + * (load_ubo, load_ssbo, store_ssbo, load_image, store_image, etc...). + * + * If the source is constant, then this is the binding table index. If we're + * going through a resource_intel intel intrinsic, then we need to check + * src[1] of that intrinsic. + */ static inline unsigned brw_nir_ubo_surface_index_get_bti(nir_src src) { @@ -155,8 +162,19 @@ brw_nir_ubo_surface_index_get_bti(nir_src src) assert(src.ssa->parent_instr->type == nir_instr_type_intrinsic); nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(src.ssa->parent_instr); - assert(intrin->intrinsic == nir_intrinsic_resource_intel); - assert(nir_src_is_const(intrin->src[1])); + if (!intrin || intrin->intrinsic != nir_intrinsic_resource_intel) + return UINT32_MAX; + + /* In practice we could even drop this intrinsic because the bindless + * access always operate from a base offset coming from a push constant, so + * they can never be constant. + */ + if (nir_intrinsic_resource_access_intel(intrin) & + nir_resource_intel_bindless) + return UINT32_MAX; + + if (!nir_src_is_const(intrin->src[1])) + return UINT32_MAX; return nir_src_as_uint(intrin->src[1]); } diff --git a/src/intel/vulkan/anv_nir_push_descriptor_analysis.c b/src/intel/vulkan/anv_nir_push_descriptor_analysis.c index 2e1c75dbd75..62eb6088381 100644 --- a/src/intel/vulkan/anv_nir_push_descriptor_analysis.c +++ b/src/intel/vulkan/anv_nir_push_descriptor_analysis.c @@ -126,18 +126,17 @@ anv_nir_loads_push_desc_buffer(nir_shader *nir, if (intrin->intrinsic != nir_intrinsic_load_ubo) continue; - const nir_const_value *const_bt_idx = - nir_src_as_const_value(intrin->src[0]); - if (const_bt_idx == NULL) + const unsigned bt_idx = + brw_nir_ubo_surface_index_get_bti(intrin->src[0]); + if (bt_idx == UINT32_MAX) continue; - const unsigned bt_idx = const_bt_idx[0].u32; - const struct anv_pipeline_binding *binding = &bind_map->surface_to_descriptor[bt_idx]; if (binding->set == ANV_DESCRIPTOR_SET_DESCRIPTORS && - binding->index == push_set) + binding->index == push_set) { return true; + } } } } @@ -162,6 +161,7 @@ anv_nir_push_desc_ubo_fully_promoted(nir_shader *nir, if (push_set_layout == NULL) return 0; + /* Assume every UBO can be promoted first. */ uint32_t ubos_fully_promoted = 0; for (uint32_t b = 0; b < push_set_layout->binding_count; b++) { const struct anv_descriptor_set_binding_layout *bind_layout = @@ -174,6 +174,10 @@ anv_nir_push_desc_ubo_fully_promoted(nir_shader *nir, ubos_fully_promoted |= BITFIELD_BIT(bind_layout->descriptor_index); } + /* For each load_ubo intrinsic, if the descriptor index or the offset is + * not a constant, we could not promote to push constant. Then check the + * offset + size against the push ranges. + */ nir_foreach_function_impl(impl, nir) { nir_foreach_block(block, impl) { nir_foreach_instr(instr, block) { @@ -184,45 +188,65 @@ anv_nir_push_desc_ubo_fully_promoted(nir_shader *nir, if (intrin->intrinsic != nir_intrinsic_load_ubo) continue; - if (!brw_nir_ubo_surface_index_is_pushable(intrin->src[0])) + /* Don't check the load_ubo from descriptor buffers */ + nir_intrinsic_instr *resource = + intrin->src[0].ssa->parent_instr->type == nir_instr_type_intrinsic ? + nir_instr_as_intrinsic(intrin->src[0].ssa->parent_instr) : NULL; + if (resource == NULL || resource->intrinsic != nir_intrinsic_resource_intel) continue; - const unsigned bt_idx = - brw_nir_ubo_surface_index_get_bti(intrin->src[0]); - - /* Skip if this isn't a load from push descriptor buffer. */ - const struct anv_pipeline_binding *binding = - &bind_map->surface_to_descriptor[bt_idx]; - if (binding->set != push_set) + /* Skip load_ubo not loading from the push descriptor */ + if (nir_intrinsic_desc_set(resource) != push_set) continue; + uint32_t binding = nir_intrinsic_binding(resource); + + /* If we have indirect indexing in the binding, no push promotion + * in possible for the entire binding. + */ + if (!nir_src_is_const(resource->src[1])) { + for (uint32_t i = 0; i < push_set_layout->binding[binding].array_size; i++) { + ubos_fully_promoted &= + ~BITFIELD_BIT(push_set_layout->binding[binding].descriptor_index + i); + } + continue; + } + + const nir_const_value *const_bt_id = + nir_src_as_const_value(resource->src[1]); + uint32_t bt_id = const_bt_id[0].u32; + + const struct anv_pipeline_binding *pipe_bind = + &bind_map->surface_to_descriptor[bt_id]; + const uint32_t desc_idx = - push_set_layout->binding[binding->binding].descriptor_index; - assert(desc_idx < MAX_PUSH_DESCRIPTORS); - - bool promoted = false; + push_set_layout->binding[binding].descriptor_index; /* If the offset in the entry is dynamic, we can't tell if * promoted or not. */ const nir_const_value *const_load_offset = nir_src_as_const_value(intrin->src[1]); - if (const_load_offset != NULL) { - /* Check if the load was promoted to a push constant. */ - const unsigned load_offset = const_load_offset[0].u32; - const int load_bytes = nir_intrinsic_dest_components(intrin) * - (intrin->def.bit_size / 8); + if (const_load_offset == NULL) { + ubos_fully_promoted &= ~BITFIELD_BIT(desc_idx); + continue; + } - for (unsigned i = 0; i < ARRAY_SIZE(bind_map->push_ranges); i++) { - if (bind_map->push_ranges[i].set == binding->set && - bind_map->push_ranges[i].index == desc_idx && - bind_map->push_ranges[i].start * 32 <= load_offset && - (bind_map->push_ranges[i].start + - bind_map->push_ranges[i].length) * 32 >= - (load_offset + load_bytes)) { - promoted = true; - break; - } + /* Check if the load was promoted to a push constant. */ + const unsigned load_offset = const_load_offset[0].u32; + const int load_bytes = nir_intrinsic_dest_components(intrin) * + (intrin->def.bit_size / 8); + + bool promoted = false; + for (unsigned i = 0; i < ARRAY_SIZE(bind_map->push_ranges); i++) { + if (bind_map->push_ranges[i].set == pipe_bind->set && + bind_map->push_ranges[i].index == desc_idx && + bind_map->push_ranges[i].start * 32 <= load_offset && + (bind_map->push_ranges[i].start + + bind_map->push_ranges[i].length) * 32 >= + (load_offset + load_bytes)) { + promoted = true; + break; } } diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c index fe08cf44b71..fcf57756c3a 100644 --- a/src/intel/vulkan/anv_pipeline.c +++ b/src/intel/vulkan/anv_pipeline.c @@ -1728,7 +1728,7 @@ anv_pipeline_account_shader(struct anv_pipeline *pipeline, if (shader->push_desc_info.used_set_buffer) { pipeline->use_push_descriptor_buffer |= - BITFIELD_BIT(mesa_to_vk_shader_stage(shader->stage)); + mesa_to_vk_shader_stage(shader->stage); } if (shader->push_desc_info.used_descriptors & ~shader->push_desc_info.fully_promoted_ubo_descriptors)