Re: [Mesa-dev] [PATCH 19/23] glsl: Add image built-in function generator.
Kenneth Graunke kenn...@whitecape.org writes:
On 11/26/2013 12:02 AM, Francisco Jerez wrote:
[snip]
+ add_image_function(imageLoad,
+ image_builtin_builder(*this)
+ .emit_stub(__intrinsic_image_load)
+ .has_return()
+ .has_vector_data_type()
+ .has_float_data_type()
+ .read_only());
I agree with Paul...I'm not a huge fan of using this pattern here.
Using true/false, i.e.
add_image_function(imageLoad, true, true, true, true, false, ...);
is clearly awful.
But what about using a flags bitfield? Something like:
enum image_function_flags {
RETURNS_DATA= (1 0),
HAS_VECTOR_DATA_TYPE= (1 1),
SUPPORTS_FLOATING_POINT_IMAGES = (1 2),
READ_ONLY = (1 3),
EMIT_STUB = (1 4),
};
and then:
void
add_image_function(const char *name,
unsigned num_arguments,
uint32_t flags);
i.e.
add_image_function(imageLoad, 0,
RETURNS_DATA |
HAS_VECTOR_DATA_TYPE |
SUPPORTS_FLOATING_POINT_IMAGES |
READ_ONLY);
That remains readable, like your code, but follows an extremely common
pattern we already use all over the place.
That would be a reasonable solution too, I'll have a look into it.
Thanks.
--Ken
pgpGtZjz9aAOb.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 19/23] glsl: Add image built-in function generator.
Paul Berry stereotype...@gmail.com writes: [...] 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(). I've fixed this one issue (and the other minor problems you pointed out) here [1]. - 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. Sorry but I don't see the point, the code looks considerably easier to follow passing parameters this way rather than using anonymous parameters because of the many arguments which are used or not in different combinations depending on the intrinsic. Using fluent parameter passing was an after-the-fact change motivated by the difficulty of verifying the correctness of the built-in definitions, which are obviously correct structured this way. I've actually had what you describe in front of me already and this is cleaner [though that might well be a matter of personal taste]. I'm not planning on going back to anonymous arguments and static arrays. Thanks. [1] http://cgit.freedesktop.org/~currojerez/mesa/commit/?h=image-load-storeid=02d4e6ee0f7665f87564ee372317fc530e0e0ef6 pgpmYLYsl2sTP.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 19/23] glsl: Add image built-in function generator.
On 11/26/2013 12:02 AM, Francisco Jerez wrote:
[snip]
+ add_image_function(imageLoad,
+ image_builtin_builder(*this)
+ .emit_stub(__intrinsic_image_load)
+ .has_return()
+ .has_vector_data_type()
+ .has_float_data_type()
+ .read_only());
I agree with Paul...I'm not a huge fan of using this pattern here.
Using true/false, i.e.
add_image_function(imageLoad, true, true, true, true, false, ...);
is clearly awful.
But what about using a flags bitfield? Something like:
enum image_function_flags {
RETURNS_DATA= (1 0),
HAS_VECTOR_DATA_TYPE= (1 1),
SUPPORTS_FLOATING_POINT_IMAGES = (1 2),
READ_ONLY = (1 3),
EMIT_STUB = (1 4),
};
and then:
void
add_image_function(const char *name,
unsigned num_arguments,
uint32_t flags);
i.e.
add_image_function(imageLoad, 0,
RETURNS_DATA |
HAS_VECTOR_DATA_TYPE |
SUPPORTS_FLOATING_POINT_IMAGES |
READ_ONLY);
That remains readable, like your code, but follows an extremely common
pattern we already use all over the place.
--Ken
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 19/23] glsl: Add image built-in function generator.
On 26 November 2013 00:02, Francisco Jerez curroje...@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 =
+
