[Mesa-dev] [PATCH v3 (part2) 09/56] glsl: Add parser/compiler support for unsized array's length()
Iago Toral
itoral at igalia.com
Wed Aug 5 04:57:01 PDT 2015
On Wed, 2015-08-05 at 13:38 +0300, Francisco Jerez wrote:
> Iago Toral <itoral at igalia.com> writes:
>
> > On Tue, 2015-08-04 at 16:04 -0700, Jordan Justen wrote:
> >> On 2015-08-04 15:12:06, Jordan Justen wrote:
> >> > On 2015-07-14 00:46:11, Iago Toral Quiroga wrote:
> >> > > From: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
> >> > >
> >> > > It also creates unop and triop expressions to tell the driver to
> >> > > calculate the unsized array length.
> >> > >
> >> > > It is needed two expressions to do the calculation:
> >> >
> >> > "Two expressions are needed to do the calculation:"
> >> >
> >> > > * The unop expression saves the ir_rvalue* whose length should be
> >> > > calculated.
> >> > > * Afterwards, this unop is going to be processed by a lowering pass
> >> > > that will convert it to a triop that includes the block index,
> >> >
> >> > line 'that' up with 'Afterwards'.
> >> >
> >> > > offset of the variable inside the shader storage block and the array
> >> > > stride. All of them are needed for length() calculation following
> >> > > GL_ARB_shader_storage_buffer spec:
> >> > >
> >> > > array.length() =
> >> > > max((buffer_object_size - offset_of_array) / stride_of_array, 0)
> >> > >
> >> > > Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
> >> > > ---
> >> > > src/glsl/hir_field_selection.cpp | 15 +++++++++++----
> >> > > src/glsl/ir.cpp | 9 +++++++++
> >> > > src/glsl/ir.h | 19 ++++++++++++++++++-
> >>
> >> Hmm, I added an r-b below, but I had a question.
> >>
> >> Rather than IR nodes, should this be handled with intrinsics like the
> >> SSBO references?
> >
> > I think that in this case it is not needed. The reason why we wanted to
> > avoid using expressions for ssbo loads is that they were not constant
> > (i.e. the same ssbo load could return different values at different
> > places in the shader program). However, my understanding is that calling
> > length() on unsized array always returns the same value. By looking at
> > the formula referenced above, the size of an unsized array is given by
> > taking the available space at the tail of the buffer (which is a fixed
> > constant value that won't change during shader execution), remove the
> > offset of the unsized array into the buffer (which is also fixed) and
> > divide by the stride (also fixed). So in this case it looks like using
> > expressions is exactly what we want.
> >
>
> I guess it would be nice to implement it as an intrinsic for consistency
> with the other SSBO operations, but I guess an expression node will work
> in practice too for now for the reasons you mention.
>
> There's one thing that seems rather non-orthogonal to me. Is there any
> reason you are hardcoding the array length formula into the instruction?
> It seems weird to have an instruction that calculates the length of an
> unsized array while the back-end basically knows nothing about the
> individual variables part of the SSBO, because they've all been lowered
> to an offset+size range within a single SSBO buffer already. The same
> goes for the VS_OPCODE_UNSIZED_ARRAY_LENGTH back-end instruction that
> knows nothing about unsized arrays, it just returns the size of the
> whole buffer.
>
> AFAICT it would simplify both the IR and the back-end implementations if
> you had a simple "get_ssbo_size" intrinsic that would take a single SSBO
> index argument and return the total size of the buffer. That would help
> because otherwise all back-ends are going to need to re-implement
> basically the same formula.
Yep, that makes more sense to me as well. For that formula, the IR
already knows the offset and the stride, it only needs the total size of
the ssbo, and that's the only thing we should have to implement in the
backends.
That said, I see no reason for that to be an intrinsic, it can perfectly
be a unop expression since it returns a constant value, pretty much like
UBO loads, which should be even easier than an intrinsic (mostly because
lowering to an intrinsic involves creating function signatures, etc).
Iago
> > Iago
> >
> >> -Jordan
> >>
> >> > > src/glsl/ir_validate.cpp | 13 +++++++++++++
> >> > > src/glsl/link_uniforms.cpp | 8 +++++++-
> >> > > .../drivers/dri/i965/brw_fs_channel_expressions.cpp | 2 ++
> >> > > src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 8 ++++++++
> >> > > src/mesa/program/ir_to_mesa.cpp | 2 ++
> >> > > src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 5 +++++
> >> > > 9 files changed, 75 insertions(+), 6 deletions(-)
> >> > >
> >> > > diff --git a/src/glsl/hir_field_selection.cpp b/src/glsl/hir_field_selection.cpp
> >> > > index 0fa9768..fedbef0 100644
> >> > > --- a/src/glsl/hir_field_selection.cpp
> >> > > +++ b/src/glsl/hir_field_selection.cpp
> >> > > @@ -71,10 +71,17 @@ _mesa_ast_field_selection_to_hir(const ast_expression *expr,
> >> > > _mesa_glsl_error(&loc, state, "length method takes no arguments");
> >> > >
> >> > > if (op->type->is_array()) {
> >> > > - if (op->type->is_unsized_array())
> >> > > - _mesa_glsl_error(&loc, state, "length called on unsized array");
> >> > > -
> >> > > - result = new(ctx) ir_constant(op->type->array_size());
> >> > > + if (op->type->is_unsized_array()) {
> >> > > + if (!state->ARB_shader_storage_buffer_object_enable) {
> >> > > + _mesa_glsl_error(&loc, state, "length called on unsized array"
> >> > > + " only available with "
> >> > > + "ARB_shader_storage_buffer_object");
> >> > > + }
> >> > > + /* Calculate length of an unsized array in run-time */
> >> > > + result = new(ctx) ir_expression(ir_unop_ssbo_unsized_array_length, op);
> >> > > + } else {
> >> > > + result = new(ctx) ir_constant(op->type->array_size());
> >> > > + }
> >> > > } else if (op->type->is_vector()) {
> >> > > if (state->ARB_shading_language_420pack_enable) {
> >> > > /* .length() returns int. */
> >> > > diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
> >> > > index 390e8f3..48c91ee 100644
> >> > > --- a/src/glsl/ir.cpp
> >> > > +++ b/src/glsl/ir.cpp
> >> > > @@ -340,6 +340,9 @@ ir_expression::ir_expression(int op, ir_rvalue *op0)
> >> > > this->type = glsl_type::get_instance(GLSL_TYPE_INT,
> >> > > op0->type->vector_elements, 1);
> >> > > break;
> >> > > + case ir_unop_ssbo_unsized_array_length:
> >> > > + this->type = glsl_type::int_type;
> >> > > + break;
> >> > >
> >> > > default:
> >> > > assert(!"not reached: missing automatic type setup for ir_expression");
> >> > > @@ -471,6 +474,10 @@ ir_expression::ir_expression(int op, ir_rvalue *op0, ir_rvalue *op1,
> >> > > this->type = op1->type;
> >> > > break;
> >> > >
> >> > > + case ir_triop_ssbo_unsized_array_length:
> >> > > + this->type = glsl_type::int_type;
> >> > > + break;
> >> > > +
> >> > > default:
> >> > > assert(!"not reached: missing automatic type setup for ir_expression");
> >> > > this->type = glsl_type::float_type;
> >> > > @@ -569,6 +576,7 @@ static const char *const operator_strs[] = {
> >> > > "frexp_exp",
> >> > > "noise",
> >> > > "interpolate_at_centroid",
> >> > > + "ssbo_unsized_array_length_parser",
> >> > > "+",
> >> > > "-",
> >> > > "*",
> >> > > @@ -609,6 +617,7 @@ static const char *const operator_strs[] = {
> >> > > "csel",
> >> > > "bfi",
> >> > > "bitfield_extract",
> >> > > + "ssbo_unsized_array_length",
> >> > > "vector_insert",
> >> > > "bitfield_insert",
> >> > > "vector",
> >> > > diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> >> > > index e677b89..b2e94a7 100644
> >> > > --- a/src/glsl/ir.h
> >> > > +++ b/src/glsl/ir.h
> >> > > @@ -1406,9 +1406,17 @@ enum ir_expression_operation {
> >> > > ir_unop_interpolate_at_centroid,
> >> > >
> >> > > /**
> >> > > + * Calculate length of an unsized array inside a buffer block.
> >> > > + * This opcode is going to be replaced in a lowering pass inside the linker.
> >> >
> >> > line length too long
> >> >
> >> > > + *
> >> > > + * operand0 is the unsized array's ir_value for the calculation of its lenght.
> >> >
> >> > line length too long
> >> >
> >> > also, typo in 'length'
> >> >
> >> > Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>
> >> >
> >> > > + */
> >> > > + ir_unop_ssbo_unsized_array_length,
> >> > > +
> >> > > + /**
> >> > > * A sentinel marking the last of the unary operations.
> >> > > */
> >> > > - ir_last_unop = ir_unop_interpolate_at_centroid,
> >> > > + ir_last_unop = ir_unop_ssbo_unsized_array_length,
> >> > >
> >> > > ir_binop_add,
> >> > > ir_binop_sub,
> >> > > @@ -1580,6 +1588,15 @@ enum ir_expression_operation {
> >> > > ir_triop_bitfield_extract,
> >> > >
> >> > > /**
> >> > > + * Calculate length of an unsized array inside a buffer block.
> >> > > + *
> >> > > + * operand0 is the ir_constant buffer block index in the linked shader.
> >> > > + * operand1 is a byte offset of the unsized array inside the buffer block
> >> > > + * operand2 is the array stride if the unsized array were sized.
> >> > > + */
> >> > > + ir_triop_ssbo_unsized_array_length,
> >> > > +
> >> > > + /**
> >> > > * Generate a value with one field of a vector changed
> >> > > *
> >> > > * operand0 is the vector
> >> > > diff --git a/src/glsl/ir_validate.cpp b/src/glsl/ir_validate.cpp
> >> > > index 684bef2..728163f 100644
> >> > > --- a/src/glsl/ir_validate.cpp
> >> > > +++ b/src/glsl/ir_validate.cpp
> >> > > @@ -409,6 +409,12 @@ ir_validate::visit_leave(ir_expression *ir)
> >> > > assert(ir->operands[0]->type->is_float());
> >> > > break;
> >> > >
> >> > > + case ir_unop_ssbo_unsized_array_length:
> >> > > + assert(ir->type == glsl_type::int_type);
> >> > > + assert(ir->operands[0]->type->is_array());
> >> > > + assert(ir->operands[0]->type->is_unsized_array());
> >> > > + break;
> >> > > +
> >> > > case ir_unop_d2f:
> >> > > assert(ir->operands[0]->type->base_type == GLSL_TYPE_DOUBLE);
> >> > > assert(ir->type->base_type == GLSL_TYPE_FLOAT);
> >> > > @@ -654,6 +660,13 @@ ir_validate::visit_leave(ir_expression *ir)
> >> > > assert(ir->operands[3]->type == glsl_type::int_type);
> >> > > break;
> >> > >
> >> > > + case ir_triop_ssbo_unsized_array_length:
> >> > > + assert(ir->type == glsl_type::int_type);
> >> > > + assert(ir->operands[0]->type == glsl_type::uint_type);
> >> > > + assert(ir->operands[1]->type == glsl_type::uint_type);
> >> > > + assert(ir->operands[2]->type == glsl_type::uint_type);
> >> > > + break;
> >> > > +
> >> > > case ir_quadop_vector:
> >> > > /* The vector operator collects some number of scalars and generates a
> >> > > * vector from them.
> >> > > diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> >> > > index e786ddc..131e76c 100644
> >> > > --- a/src/glsl/link_uniforms.cpp
> >> > > +++ b/src/glsl/link_uniforms.cpp
> >> > > @@ -221,7 +221,13 @@ program_resource_visitor::recursion(const glsl_type *t, char **name,
> >> > > if (record_type == NULL && t->fields.array->is_record())
> >> > > record_type = t->fields.array;
> >> > >
> >> > > - for (unsigned i = 0; i < t->length; i++) {
> >> > > + unsigned length = t->length;
> >> > > + /* Shader storage block unsized arrays: add subscript [0] to variable
> >> > > + * names */
> >> > > + if (t->is_unsized_array())
> >> > > + length = 1;
> >> > > +
> >> > > + for (unsigned i = 0; i < length; i++) {
> >> > > size_t new_length = name_length;
> >> > >
> >> > > /* Append the subscript to the current variable name */
> >> > > diff --git a/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp b/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp
> >> > > index d0f6122..fb06c10 100644
> >> > > --- a/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp
> >> > > +++ b/src/mesa/drivers/dri/i965/brw_fs_channel_expressions.cpp
> >> > > @@ -378,6 +378,7 @@ ir_channel_expressions_visitor::visit_leave(ir_assignment *ir)
> >> > > }
> >> > >
> >> > > case ir_binop_ubo_load:
> >> > > + case ir_triop_ssbo_unsized_array_length:
> >> > > unreachable("not yet supported");
> >> > >
> >> > > case ir_triop_fma:
> >> > > @@ -429,6 +430,7 @@ ir_channel_expressions_visitor::visit_leave(ir_assignment *ir)
> >> > > case ir_triop_vector_insert:
> >> > > case ir_quadop_bitfield_insert:
> >> > > case ir_quadop_vector:
> >> > > + case ir_unop_ssbo_unsized_array_length:
> >> > > unreachable("should have been lowered");
> >> > >
> >> > > case ir_unop_unpack_half_2x16_split_x:
> >> > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> >> > > index 0c8b0bd..d95b7c8 100644
> >> > > --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> >> > > +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> >> > > @@ -1555,6 +1555,10 @@ vec4_visitor::visit(ir_expression *ir)
> >> > > case ir_unop_noise:
> >> > > unreachable("not reached: should be handled by lower_noise");
> >> > >
> >> > > + case ir_unop_ssbo_unsized_array_length:
> >> > > + unreachable("not reached: should be handled by lower_ubo_reference");
> >> > > + break;
> >> > > +
> >> > > case ir_binop_add:
> >> > > emit(ADD(result_dst, op[0], op[1]));
> >> > > break;
> >> > > @@ -1924,6 +1928,10 @@ vec4_visitor::visit(ir_expression *ir)
> >> > > emit(BFE(result_dst, op[2], op[1], op[0]));
> >> > > break;
> >> > >
> >> > > + case ir_triop_ssbo_unsized_array_length:
> >> > > + unreachable("not reached: not implemented");
> >> > > + break;
> >> > > +
> >> > > case ir_triop_vector_insert:
> >> > > unreachable("should have been lowered by lower_vector_insert");
> >> > >
> >> > > diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
> >> > > index 2bd212e..ff7115c 100644
> >> > > --- a/src/mesa/program/ir_to_mesa.cpp
> >> > > +++ b/src/mesa/program/ir_to_mesa.cpp
> >> > > @@ -1342,9 +1342,11 @@ ir_to_mesa_visitor::visit(ir_expression *ir)
> >> > > case ir_unop_dFdx_fine:
> >> > > case ir_unop_dFdy_coarse:
> >> > > case ir_unop_dFdy_fine:
> >> > > + case ir_triop_ssbo_unsized_array_length:
> >> > > assert(!"not supported");
> >> > > break;
> >> > >
> >> > > + case ir_unop_ssbo_unsized_array_length:
> >> > > case ir_quadop_vector:
> >> > > /* This operation should have already been handled.
> >> > > */
> >> > > diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> >> > > index 25e30c7..244d897 100644
> >> > > --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> >> > > +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> >> > > @@ -2172,10 +2172,15 @@ glsl_to_tgsi_visitor::visit(ir_expression *ir)
> >> > > case ir_triop_vector_insert:
> >> > > case ir_binop_carry:
> >> > > case ir_binop_borrow:
> >> > > + case ir_unop_ssbo_unsized_array_length:
> >> > > /* This operation is not supported, or should have already been handled.
> >> > > */
> >> > > assert(!"Invalid ir opcode in glsl_to_tgsi_visitor::visit()");
> >> > > break;
> >> > > +
> >> > > + case ir_triop_ssbo_unsized_array_length:
> >> > > + assert(!"Not implemented yet");
> >> > > + break;
> >> > > }
> >> > >
> >> > > this->result = result_src;
> >> > > --
> >> > > 1.9.1
> >> > >
> >>
More information about the mesa-dev
mailing list