<div dir="ltr">On 26 November 2013 00:02, Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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; </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +} + /** @} */ /******************************************************************************/ 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; + } </blockquote><div> </div><div>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". </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + 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; }; </blockquote><div> </div><div>I see a few downsides to this approach: </div><div>- 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(). </div><div>- 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. </div><div>- 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. </div><div>Here's what I would recommend doing instead: </div><div>- Change image_builtin_builder to a plain struct, and remove the fluent setter functions for setting its fields. </div><div> - 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. </div><div> </div><div>- At toplevel in buildin_functions.cpp, declare a static const array of image_builtin_builder objects, one for each image function. </div><div>- 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. </div><div>- 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. </div></div></div></div>