On Tue, 2016-01-19 at 01:50 -0500, Ilia Mirkin wrote:
> Currently any access params (coherent/volatile/restrict) are being lost
> when lowering to the ssbo load/store intrinsics. Keep track of the
> variable being used, and bake its access params in as the last arg of
> the load/store intrinsics.
>
> Sometimes the variable being accessed is the actual block itself, in the
> case of named blocks. In that case, retrieve the field struct and use
> its parameters.
I found this paragraph about a bit confusing. How about this instead?:
If the variable is accessed via an instance block, then 'variable'
points to the instance block variable and not the field inside the
instance block that we are accessing. In order to check access
parameters for the field itself we need to detect this case and keep
track of the corresponding field struct so we can extract the specific
field access information from there instead.
>
> Signed-off-by: Ilia Mirkin <imir...@alum.mit.edu>
> Reviewed-by: Marek Olšák <marek.ol...@amd.com> (v1)
> Reviewed-by: Iago Toral Quiroga <ito...@igalia.com> (v1)
>
> v1 -> v2: add tracking of struct field
> ---
>
> (Only resent these two patches... not the whole series.)
>
> src/glsl/lower_buffer_access.cpp | 8 +++++++-
> src/glsl/lower_buffer_access.h | 1 +
> src/glsl/lower_shared_reference.cpp | 6 +++---
> src/glsl/lower_ubo_reference.cpp | 32 +++++++++++++++++++++++++++++++-
> src/glsl/nir/shader_enums.h | 10 ++++++++++
> 5 files changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/src/glsl/lower_buffer_access.cpp
> b/src/glsl/lower_buffer_access.cpp
> index f8c8d14..0b0bd31 100644
> --- a/src/glsl/lower_buffer_access.cpp
> +++ b/src/glsl/lower_buffer_access.cpp
> @@ -327,11 +327,14 @@ lower_buffer_access::setup_buffer_access(void *mem_ctx,
> unsigned *const_offset,
> bool *row_major,
> int *matrix_columns,
> + const glsl_struct_field
> **struct_field,
> unsigned packing)
> {
> *offset = new(mem_ctx) ir_constant(0u);
> *row_major = is_dereferenced_thing_row_major(deref);
> *matrix_columns = 1;
> + if (struct_field)
> + *struct_field = NULL;
Not sure how I feel about this... I think I'd rather initialize
struct_field to NULL in lower_ubo_reference and let this function assign
a value only if there is one to assign.
> /* Calculate the offset to the start of the region of the UBO
> * dereferenced by *rvalue. This may be a variable offset if an
> @@ -442,8 +445,11 @@ lower_buffer_access::setup_buffer_access(void *mem_ctx,
> intra_struct_offset = glsl_align(intra_struct_offset,
> field_align);
>
> if (strcmp(struct_type->fields.structure[i].name,
> - deref_record->field) == 0)
> + deref_record->field) == 0) {
> + if (struct_field)
> + *struct_field = &struct_type->fields.structure[i];
> break;
> + }
>
> if (packing == GLSL_INTERFACE_PACKING_STD430)
> intra_struct_offset += type->std430_size(field_row_major);
> diff --git a/src/glsl/lower_buffer_access.h b/src/glsl/lower_buffer_access.h
> index cc4614e..8772bdb 100644
> --- a/src/glsl/lower_buffer_access.h
> +++ b/src/glsl/lower_buffer_access.h
> @@ -57,6 +57,7 @@ public:
> void setup_buffer_access(void *mem_ctx, ir_variable *var, ir_rvalue
> *deref,
> ir_rvalue **offset, unsigned *const_offset,
> bool *row_major, int *matrix_columns,
> + const glsl_struct_field **struct_field,
> unsigned packing);
> };
>
> diff --git a/src/glsl/lower_shared_reference.cpp
> b/src/glsl/lower_shared_reference.cpp
> index 533cd92..1249969 100644
> --- a/src/glsl/lower_shared_reference.cpp
> +++ b/src/glsl/lower_shared_reference.cpp
> @@ -142,7 +142,7 @@ lower_shared_reference_visitor::handle_rvalue(ir_rvalue
> **rvalue)
>
> setup_buffer_access(mem_ctx, var, deref,
> &offset, &const_offset,
> - &row_major, &matrix_columns, packing);
> + &row_major, &matrix_columns, NULL, packing);
>
> /* Now that we've calculated the offset to the start of the
> * dereference, walk over the type and emit loads into a temporary.
> @@ -210,7 +210,7 @@
> lower_shared_reference_visitor::handle_assignment(ir_assignment *ir)
>
> setup_buffer_access(mem_ctx, var, deref,
> &offset, &const_offset,
> - &row_major, &matrix_columns, packing);
> + &row_major, &matrix_columns, NULL, packing);
>
> deref = new(mem_ctx) ir_dereference_variable(store_var);
>
> @@ -370,7 +370,7 @@
> lower_shared_reference_visitor::lower_shared_atomic_intrinsic(ir_call *ir)
>
> setup_buffer_access(mem_ctx, var, deref,
> &offset, &const_offset,
> - &row_major, &matrix_columns, packing);
> + &row_major, &matrix_columns, NULL, packing);
>
> assert(offset);
> assert(!row_major);
> diff --git a/src/glsl/lower_ubo_reference.cpp
> b/src/glsl/lower_ubo_reference.cpp
> index a172054..d9d2928 100644
> --- a/src/glsl/lower_ubo_reference.cpp
> +++ b/src/glsl/lower_ubo_reference.cpp
> @@ -60,6 +60,7 @@ public:
> bool *row_major,
> int *matrix_columns,
> unsigned packing);
> + uint32_t ssbo_access_params();
> ir_expression *ubo_load(void *mem_ctx, const struct glsl_type *type,
> ir_rvalue *offset);
> ir_call *ssbo_load(void *mem_ctx, const struct glsl_type *type,
> @@ -104,6 +105,8 @@ public:
>
> struct gl_shader *shader;
> struct gl_uniform_buffer_variable *ubo_var;
> + const struct glsl_struct_field *struct_field;
> + ir_variable *variable;
> ir_rvalue *uniform_block;
> bool progress;
> };
> @@ -289,7 +292,7 @@ lower_ubo_reference_visitor::setup_for_load_or_store(void
> *mem_ctx,
> *const_offset = ubo_var->Offset;
>
> setup_buffer_access(mem_ctx, var, deref, offset, const_offset, row_major,
> - matrix_columns, packing);
> + matrix_columns, &this->struct_field, packing);
> }
>
> void
> @@ -317,6 +320,7 @@ lower_ubo_reference_visitor::handle_rvalue(ir_rvalue
> **rvalue)
> this->buffer_access_type =
> var->is_in_shader_storage_block() ?
> ssbo_load_access : ubo_load_access;
> + this->variable = var;
>
> /* Compute the offset to the start if the dereference as well as other
> * information we need to configure the write
> @@ -370,6 +374,19 @@ shader_storage_buffer_object(const
> _mesa_glsl_parse_state *state)
> return state->ARB_shader_storage_buffer_object_enable;
> }
>
> +uint32_t
> +lower_ubo_reference_visitor::ssbo_access_params()
> +{
This expects "variable" to be assigned before we get here. Since we have
to take care of that in different places I think it would be good to add
an assertion here to make sure that we have indeed assigned it. That
would require that we initialize variable to NULL in the constructor as
well to make this useful.
> + if (variable->is_interface_instance())
Maybe add an assert here as well to make sure that we have assigned a
valid struct_field in this case? This would be another point in favor of
initializing this to NULL in lower_ubo_reference.
I realize that we are not giving the NULL initialization treatment to
ubo_var or uniform_block. We probably should do that too, but maybe in
these two cases it is less relevant because they are assigned only in
one place and in a function that will always be called, since it is the
one that computes the offset being accessed anyway...
Reviewed-by: Iago Toral Quiroga <ito...@igalia.com>
> + return ((struct_field->image_coherent ? ACCESS_COHERENT : 0) |
> + (struct_field->image_restrict ? ACCESS_RESTRICT : 0) |
> + (struct_field->image_volatile ? ACCESS_VOLATILE : 0));
> + else
> + return ((variable->data.image_coherent ? ACCESS_COHERENT : 0) |
> + (variable->data.image_restrict ? ACCESS_RESTRICT : 0) |
> + (variable->data.image_volatile ? ACCESS_VOLATILE : 0));
> +}
> +
> ir_call *
> lower_ubo_reference_visitor::ssbo_store(void *mem_ctx,
> ir_rvalue *deref,
> @@ -394,6 +411,10 @@ lower_ubo_reference_visitor::ssbo_store(void *mem_ctx,
> ir_variable(glsl_type::uint_type, "write_mask" , ir_var_function_in);
> sig_params.push_tail(writemask_ref);
>
> + ir_variable *access_ref = new(mem_ctx)
> + ir_variable(glsl_type::uint_type, "access" , ir_var_function_in);
> + sig_params.push_tail(access_ref);
> +
> ir_function_signature *sig = new(mem_ctx)
> ir_function_signature(glsl_type::void_type,
> shader_storage_buffer_object);
> assert(sig);
> @@ -408,6 +429,7 @@ lower_ubo_reference_visitor::ssbo_store(void *mem_ctx,
> call_params.push_tail(offset->clone(mem_ctx, NULL));
> call_params.push_tail(deref->clone(mem_ctx, NULL));
> call_params.push_tail(new(mem_ctx) ir_constant(write_mask));
> + call_params.push_tail(new(mem_ctx) ir_constant(ssbo_access_params()));
> return new(mem_ctx) ir_call(sig, NULL, &call_params);
> }
>
> @@ -426,6 +448,10 @@ lower_ubo_reference_visitor::ssbo_load(void *mem_ctx,
> ir_variable(glsl_type::uint_type, "offset_ref" , ir_var_function_in);
> sig_params.push_tail(offset_ref);
>
> + ir_variable *access_ref = new(mem_ctx)
> + ir_variable(glsl_type::uint_type, "access" , ir_var_function_in);
> + sig_params.push_tail(access_ref);
> +
> ir_function_signature *sig =
> new(mem_ctx) ir_function_signature(type, shader_storage_buffer_object);
> assert(sig);
> @@ -444,6 +470,7 @@ lower_ubo_reference_visitor::ssbo_load(void *mem_ctx,
> exec_list call_params;
> call_params.push_tail(this->uniform_block->clone(mem_ctx, NULL));
> call_params.push_tail(offset->clone(mem_ctx, NULL));
> + call_params.push_tail(new(mem_ctx) ir_constant(ssbo_access_params()));
>
> return new(mem_ctx) ir_call(sig, deref_result, &call_params);
> }
> @@ -499,6 +526,7 @@ lower_ubo_reference_visitor::write_to_memory(void
> *mem_ctx,
> unsigned packing = var->get_interface_type()->interface_packing;
>
> this->buffer_access_type = ssbo_store_access;
> + this->variable = var;
>
> /* Compute the offset to the start if the dereference as well as other
> * information we need to configure the write
> @@ -678,6 +706,7 @@
> lower_ubo_reference_visitor::process_ssbo_unsized_array_length(ir_rvalue
> **rvalu
> int unsized_array_stride = calculate_unsized_array_stride(deref, packing);
>
> this->buffer_access_type = ssbo_unsized_array_length_access;
> + this->variable = var;
>
> /* Compute the offset to the start if the dereference as well as other
> * information we need to calculate the length.
> @@ -910,6 +939,7 @@
> lower_ubo_reference_visitor::lower_ssbo_atomic_intrinsic(ir_call *ir)
> unsigned packing = var->get_interface_type()->interface_packing;
>
> this->buffer_access_type = ssbo_atomic_access;
> + this->variable = var;
>
> setup_for_load_or_store(mem_ctx, var, deref,
> &offset, &const_offset,
> diff --git a/src/glsl/nir/shader_enums.h b/src/glsl/nir/shader_enums.h
> index c747464..19610c9 100644
> --- a/src/glsl/nir/shader_enums.h
> +++ b/src/glsl/nir/shader_enums.h
> @@ -535,6 +535,16 @@ enum gl_frag_depth_layout
> FRAG_DEPTH_LAYOUT_UNCHANGED
> };
>
> +/**
> + * \brief Buffer access qualifiers
> + */
> +enum gl_buffer_access_qualifier
> +{
> + ACCESS_COHERENT = 1,
> + ACCESS_RESTRICT = 2,
> + ACCESS_VOLATILE = 4,
> +};
> +
> #ifdef __cplusplus
> } /* extern "C" */
> #endif
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
