[Mesa-dev] [PATCH 19/23] glsl: Add image built-in function generator.
Paul Berry
stereotype441 at gmail.com
Wed Dec 11 13:53:15 PST 2013
On 26 November 2013 00:02, Francisco Jerez <currojerez at riseup.net> wrote:
> Because of the combinatorial explosion of different image built-ins
> with different image dimensionalities and base data types, enumerating
> all the 242 possibilities would be annoying and a waste of .text
> space. Instead use a special path in the built-in builder that loops
> over all the known image types.
> ---
> src/glsl/builtin_functions.cpp | 378
> +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 378 insertions(+)
>
> diff --git a/src/glsl/builtin_functions.cpp
> b/src/glsl/builtin_functions.cpp
> index a1a338d..760e264 100644
> --- a/src/glsl/builtin_functions.cpp
> +++ b/src/glsl/builtin_functions.cpp
> @@ -334,12 +334,20 @@ shader_atomic_counters(const _mesa_glsl_parse_state
> *state)
> return state->ARB_shader_atomic_counters_enable;
> }
>
> +static bool
> +shader_image_load_store(const _mesa_glsl_parse_state *state)
> +{
> + return state->ARB_shader_image_load_store_enable;
>
As in the previous patch, let's make this "state->is_version(420, 0) ||
state->ARB_shader_image_load_store_enable" so that the right thing will
happen when we get to GLSL version 4.20.
> +}
> +
> /** @} */
>
>
> /******************************************************************************/
>
> namespace {
>
> +class image_builtin_builder;
> +
> /**
> * builtin_builder: A singleton object representing the core of the
> built-in
> * function module.
> @@ -406,6 +414,13 @@ private:
> /** Create a new function and add the given signatures. */
> void add_function(const char *name, ...);
>
> + /**
> + * Create a new function calling \param func for each known image
> + * type to generate its signatures.
> + */
> + void add_image_function(const char *name,
> + const image_builtin_builder &func);
> +
> ir_function_signature *new_sig(const glsl_type *return_type,
> builtin_available_predicate avail,
> int num_params, ...);
> @@ -569,6 +584,11 @@ private:
> ir_function_signature *_atomic_op(const char *intrinsic,
> builtin_available_predicate avail);
>
> + ir_function_signature *_memory_barrier_intrinsic(
> + builtin_available_predicate avail);
> + ir_function_signature *_memory_barrier(
> + builtin_available_predicate avail);
> +
> #undef B0
> #undef B1
> #undef B2
> @@ -576,6 +596,171 @@ private:
> #undef BA1
> #undef BA2
> /** @} */
> +
> + friend class image_builtin_builder;
> +};
> +
> +/**
> + * Functor that generates image load, store or atomic built-in
> + * signatures given some settings.
> + */
> +class image_builtin_builder
> +{
> +public:
> + image_builtin_builder(builtin_builder &bld)
> + : bld(bld),
> + _emit_stub(false),
> + _intrinsic_name(NULL),
> + _has_return(false),
> + _has_arguments(0),
> + _has_vector_data_type(false),
> + _has_float_data_type(false),
> + _read_only(false),
> + _write_only(false)
> + {
> + }
> +
> + /**
> + * Build a stub function that calls \c intrinsic_name forwarding
> + * arguments and return type.
> + */
> + image_builtin_builder &
> + emit_stub(const char *intrinsic_name)
> + {
> + _emit_stub = true;
> + _intrinsic_name = intrinsic_name;
> + return *this;
> + }
> +
> + image_builtin_builder &
> + has_return()
> + {
> + _has_return = true;
> + return *this;
> + }
> +
> + image_builtin_builder &
> + has_arguments(unsigned n)
> + {
> + _has_arguments = n;
> + return *this;
> + }
> +
> + image_builtin_builder &
> + has_vector_data_type()
> + {
> + _has_vector_data_type = true;
> + return *this;
> + }
> +
> + image_builtin_builder &
> + has_float_data_type()
> + {
> + _has_float_data_type = true;
> + return *this;
> + }
>
has_float_data_type is a confusing name. What this boolean really
represents is the fact that the function in question *supports* floating
point images. Functions having this property may or may not have float
point data types involved in their signatures depending on the image type.
I'd suggest changing the name to "supports_floating_point_images".
> +
> + image_builtin_builder &
> + read_only()
> + {
> + _read_only = true;
> + return *this;
> + }
> +
> + image_builtin_builder &
> + write_only()
> + {
> + _write_only = true;
> + return *this;
> + }
> +
> + /**
> + * Generate the image built-in.
> + */
> + ir_function_signature *
> + operator()(const glsl_type *image_type) const
> + {
> + if (image_type->fields.image.type == GLSL_TYPE_FLOAT &&
> + !_has_float_data_type)
> + return NULL;
> +
> + ir_function_signature *sig = create_signature(image_type);
> +
> + if (_emit_stub) {
> + ir_factory body(&sig->body, bld.mem_ctx);
> + ir_function *f =
> bld.shader->symbols->get_function(_intrinsic_name);
> +
> + if (_has_return) {
> + ir_variable *ret_val =
> + body.make_temp(sig->return_type, "_ret_val");
> + body.emit(bld.call(f, ret_val, sig->parameters));
> + body.emit(ret(ret_val));
> + } else {
> + body.emit(bld.call(f, NULL, sig->parameters));
> + }
> +
> + sig->is_defined = true;
> +
> + } else {
> + sig->is_intrinsic = true;
> + }
> +
> + return sig;
> + }
> +
> +private:
> + ir_function_signature *
> + create_signature(const glsl_type *image_type) const
> + {
> + const glsl_type *data_type =
> + glsl_type::get_instance(image_type->fields.image.type,
> + (_has_vector_data_type ? 4 : 1), 1);
> + const glsl_type *ret_type = (_has_return ? data_type :
> + glsl_type::void_type);
> +
> + /* Addressing arguments that are always present. */
> + ir_variable *image = bld.in_var(image_type, "image");
> + ir_variable *coord = bld.in_var(
> + glsl_type::ivec(image_type->coordinate_components()), "coord");
> +
> + ir_function_signature *sig =
> + bld.new_sig(ret_type, shader_image_load_store, 2, image, coord);
> +
> + /* Sample index for multisample images. */
> + if (image_type->fields.image.dimension == GLSL_IMAGE_DIM_MS)
> + sig->parameters.push_tail(
> + bld.in_var(glsl_type::int_type, "sample"));
> +
> + /* Data arguments. */
> + for (unsigned i = 0; i < _has_arguments; ++i)
> + sig->parameters.push_tail(
> + bld.in_var(data_type, ralloc_asprintf(NULL, "arg%d", i)));
> +
> + /* Set the maximal set of qualifiers allowed for this image
> + * built-in. Function calls with arguments having fewer
> + * qualifiers than present in the prototype are allowed by the
> + * spec, but not with more, i.e. this will make the compiler
> + * accept everything that needs to be accepted, and reject cases
> + * like loads from write-only or stores to read-only images.
> + */
> + image->image.read_only = _read_only;
> + image->image.write_only = _write_only;
> + image->image.coherent = true;
> + image->image._volatile = true;
> + image->image._restrict = true;
> +
> + return sig;
> + }
> +
> + builtin_builder &bld;
> + bool _emit_stub;
> + const char *_intrinsic_name;
> + bool _has_return;
> + unsigned _has_arguments;
> + bool _has_vector_data_type;
> + bool _has_float_data_type;
> + bool _read_only;
> + bool _write_only;
> };
>
I see a few downsides to this approach:
- There is unnecessary code duplication between
builtin_builder::create_intrinsics() and
builtin_builder::create_builtins(). Both of them create the same set of
functions using the same parameters, with the only difference being the
function names and whether emit_stub() is called. Furthermore, if the
names passed to emit_stub() in create_builtins() have to match the function
names used in create_intrinsics().
- builtin_builder::create_intrinsics() and
builtin_builder::create_builtins() create the same set of
image_builtin_builder objects every time they are called. I think the code
would be easier to follow if the set of image_builtin_builter objects were
declared as a static const table.
- Currently we don't have any precedent for the use of the fluent interface
style in Mesa. While I'm not opposed to it on principle, I don't think
this particular use of it is very compelling, and I'd prefer to delay
introducing it until we have a case where the benefit is clearer.
Here's what I would recommend doing instead:
- Change image_builtin_builder to a plain struct, and remove the fluent
setter functions for setting its fields.
- Remove the emit_stub boolean and instead make it an argument to
add_image_function().
- Remove the name parameter to add_image_function(), and instead store both
the intrinsic name and the builtin name as separate fields in the struct.
add_image_function() will choose which name to use based on its emit_stub
argument.
- At toplevel in buildin_functions.cpp, declare a static const array of
image_builtin_builder objects, one for each image function.
- In builtin_builder::create_intrinsics(), loop through the static array of
image_builtin_builders, calling add_image_function() on each and passing
false for emit_stub.
- In builtin_builder::create_builtins(), loop through the static array of
image_builtin_builders, calling add_image_function() on each and passing
true for emit_stub.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131211/0cfe06cc/attachment-0001.html>
More information about the mesa-dev
mailing list