<div dir="ltr">On 17 December 2013 02:10, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><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"><div class="im">Because of the combinatorial explosion of different image built-ins<br>
with different image dimensionalities and base data types, enumerating<br>
all the 242 possibilities would be annoying and a waste of .text<br>
space. Instead use a special path in the built-in builder that loops<br>
over all the known image types.<br>
<br>
</div>v2: Generate built-ins on GLSL version 4.20 too. Rename<br>
'_has_float_data_type' to '_supports_float_data_type'. Avoid<br>
duplicating enumeration of image built-ins in create_intrinsics()<br>
and create_builtins().<br>
v3: Use a more orthodox approach for passing image built-in generator<br>
parameters.<br>
---<br>
src/glsl/builtin_functions.cpp | 267 +++++++++++++++++++++++++++++++++++++++++<br>
1 file changed, 267 insertions(+)<br>
<br>
diff --git a/src/glsl/builtin_functions.cpp b/src/glsl/builtin_functions.cpp<br>
index 4251948..90a9336 100644<br>
--- a/src/glsl/builtin_functions.cpp<br>
+++ b/src/glsl/builtin_functions.cpp<br>
@@ -334,6 +334,13 @@ shader_atomic_counters(const _mesa_glsl_parse_state *state)<br>
<div class="im"> return state->ARB_shader_atomic_counters_enable;<br>
}<br>
<br>
+static bool<br>
+shader_image_load_store(const _mesa_glsl_parse_state *state)<br>
+{<br>
</div>+ return (state->is_version(420, 0) ||<br>
+ state->ARB_shader_image_load_store_enable);<br>
<div class="im">+}<br>
+<br>
/** @} */<br>
<br>
/******************************************************************************/<br>
</div>@@ -407,6 +414,33 @@ private:<br>
<div class="im"> /** Create a new function and add the given signatures. */<br>
void add_function(const char *name, ...);<br>
<br>
</div>+ enum image_function_flags {<br>
+ IMAGE_FUNCTION_EMIT_STUB = (1 << 0),<br>
+ IMAGE_FUNCTION_HAS_RETURN = (1 << 1),<br>
+ IMAGE_FUNCTION_HAS_VECTOR_DATA_TYPE = (1 << 2),<br>
+ IMAGE_FUNCTION_SUPPORTS_FLOAT_DATA_TYPE = (1 << 3),<br>
+ IMAGE_FUNCTION_READ_ONLY = (1 << 4),<br>
+ IMAGE_FUNCTION_WRITE_ONLY = (1 << 5)<br>
+ };<br>
+<br>
+ /**<br>
+ * Create a new image built-in function for all known image types.<br>
+ * \p flags is a bitfield of \c image_function_flags flags.<br>
<div class="im">+ */<br>
+ void add_image_function(const char *name,<br>
</div>+ const char *intrinsic_name,<br>
+ unsigned num_arguments,<br>
+ unsigned flags);<br>
+<br>
+ /**<br>
+ * Create new functions for all known image built-ins and types.<br>
+ * If \p glsl is \c true, use the GLSL built-in names and emit code<br>
+ * to call into the actual compiler intrinsic. If \p glsl is<br>
+ * false, emit a function prototype with no body for each image<br>
+ * intrinsic name.<br>
+ */<br>
+ void add_image_functions(bool glsl);<br>
<div class="im">+<br>
ir_function_signature *new_sig(const glsl_type *return_type,<br>
builtin_available_predicate avail,<br>
int num_params, ...);<br>
</div>@@ -570,6 +604,20 @@ private:<br>
<div class="im"> ir_function_signature *_atomic_op(const char *intrinsic,<br>
builtin_available_predicate avail);<br>
<br>
</div>+ ir_function_signature *_image_prototype(const glsl_type *image_type,<br>
+ const char *intrinsic_name,<br>
+ unsigned num_arguments,<br>
+ unsigned flags);<br>
+ ir_function_signature *_image(const glsl_type *image_type,<br>
+ const char *intrinsic_name,<br>
+ unsigned num_arguments,<br>
+ unsigned flags);<br>
<div class="im">+<br>
+ ir_function_signature *_memory_barrier_intrinsic(<br>
+ builtin_available_predicate avail);<br>
+ ir_function_signature *_memory_barrier(<br>
+ builtin_available_predicate avail);<br>
+<br>
#undef B0<br>
#undef B1<br>
#undef B2<br>
</div>@@ -684,6 +732,12 @@ builtin_builder::create_intrinsics()<br>
<div class="im"> add_function("__intrinsic_atomic_predecrement",<br>
_atomic_intrinsic(shader_atomic_counters),<br>
NULL);<br>
+<br>
</div>+ add_image_functions(false);<br>
<div class="im">+<br>
+ add_function("__intrinsic_memory_barrier",<br>
+ _memory_barrier_intrinsic(shader_image_load_store),<br>
+ NULL);<br>
}<br>
<br>
/**<br>
</div>@@ -2106,6 +2160,12 @@ builtin_builder::create_builtins()<br>
shader_atomic_counters),<br>
NULL);<br>
<br>
+ add_image_functions(true);<br>
<div class="im">+<br>
+ add_function("memoryBarrier",<br>
+ _memory_barrier(shader_image_load_store),<br>
+ NULL);<br>
+<br>
#undef F<br>
#undef FI<br>
#undef FIU<br>
</div>@@ -2139,6 +2199,120 @@ builtin_builder::add_function(const char *name, ...)<br>
shader->symbols->add_function(f);<br>
}<br>
<br>
+void<br>
+builtin_builder::add_image_function(const char *name,<br>
+ const char *intrinsic_name,<br>
+ unsigned num_arguments,<br>
+ unsigned flags)<br>
<div><div class="h5">+{<br>
+ static const glsl_type *const types[] = {<br>
+ glsl_type::image1D_type,<br>
+ glsl_type::image2D_type,<br>
+ glsl_type::image3D_type,<br>
+ glsl_type::image2DRect_type,<br>
+ glsl_type::imageCube_type,<br>
+ glsl_type::imageBuffer_type,<br>
+ glsl_type::image1DArray_type,<br>
+ glsl_type::image2DArray_type,<br>
+ glsl_type::imageCubeArray_type,<br>
+ glsl_type::image2DMS_type,<br>
+ glsl_type::image2DMSArray_type,<br>
+ glsl_type::iimage1D_type,<br>
+ glsl_type::iimage2D_type,<br>
+ glsl_type::iimage3D_type,<br>
+ glsl_type::iimage2DRect_type,<br>
+ glsl_type::iimageCube_type,<br>
+ glsl_type::iimageBuffer_type,<br>
+ glsl_type::iimage1DArray_type,<br>
+ glsl_type::iimage2DArray_type,<br>
+ glsl_type::iimageCubeArray_type,<br>
+ glsl_type::iimage2DMS_type,<br>
+ glsl_type::iimage2DMSArray_type,<br>
+ glsl_type::uimage1D_type,<br>
+ glsl_type::uimage2D_type,<br>
+ glsl_type::uimage3D_type,<br>
+ glsl_type::uimage2DRect_type,<br>
+ glsl_type::uimageCube_type,<br>
+ glsl_type::uimageBuffer_type,<br>
+ glsl_type::uimage1DArray_type,<br>
+ glsl_type::uimage2DArray_type,<br>
+ glsl_type::uimageCubeArray_type,<br>
+ glsl_type::uimage2DMS_type,<br>
+ glsl_type::uimage2DMSArray_type<br>
+ };<br>
+ ir_function *f = new(mem_ctx) ir_function(name);<br>
+<br>
</div></div>+ for (unsigned i = 0; i < Elements(types); ++i) {<br>
+ if (types[i]->fields.image.type != GLSL_TYPE_FLOAT ||<br>
+ (flags & IMAGE_FUNCTION_SUPPORTS_FLOAT_DATA_TYPE))<br>
+ f->add_signature(_image(types[i], intrinsic_name,<br>
+ num_arguments, flags));<br>
<div class="im">+ }<br>
+<br>
+ shader->symbols->add_function(f);<br>
+}<br>
+<br>
</div>+void<br>
+builtin_builder::add_image_functions(bool glsl)<br>
+{<br>
+ add_image_function(glsl ? "imageLoad" : "__intrinsic_image_load",<br>
+ "__intrinsic_image_load", 0,<br>
+ ((glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) |<br>
+ IMAGE_FUNCTION_HAS_RETURN |<br>
+ IMAGE_FUNCTION_HAS_VECTOR_DATA_TYPE |<br>
+ IMAGE_FUNCTION_SUPPORTS_FLOAT_DATA_TYPE |<br>
+ IMAGE_FUNCTION_READ_ONLY));<br>
+<br>
+ add_image_function(glsl ? "imageStore" : "__intrinsic_image_store",<br>
+ "__intrinsic_image_store", 1,<br>
+ ((glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) |<br>
+ IMAGE_FUNCTION_HAS_VECTOR_DATA_TYPE |<br>
+ IMAGE_FUNCTION_SUPPORTS_FLOAT_DATA_TYPE |<br>
+ IMAGE_FUNCTION_WRITE_ONLY));<br>
+<br>
+ add_image_function(glsl ? "imageAtomicAdd" : "__intrinsic_image_atomic_add",<br>
+ "__intrinsic_image_atomic_add", 1,<br>
+ ((glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) |<br>
+ IMAGE_FUNCTION_HAS_RETURN));<br>
+<br>
+ add_image_function(glsl ? "imageAtomicMin" : "__intrinsic_image_atomic_min",<br>
+ "__intrinsic_image_atomic_min", 1,<br>
+ ((glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) |<br>
+ IMAGE_FUNCTION_HAS_RETURN));<br>
+<br>
+ add_image_function(glsl ? "imageAtomicMax" : "__intrinsic_image_atomic_max",<br>
+ "__intrinsic_image_atomic_max", 1,<br>
+ ((glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) |<br>
+ IMAGE_FUNCTION_HAS_RETURN));<br>
+<br>
+ add_image_function(glsl ? "imageAtomicAnd" : "__intrinsic_image_atomic_and",<br>
+ "__intrinsic_image_atomic_and", 1,<br>
+ ((glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) |<br>
+ IMAGE_FUNCTION_HAS_RETURN));<br>
+<br>
+ add_image_function(glsl ? "imageAtomicOr" : "__intrinsic_image_atomic_or",<br>
+ "__intrinsic_image_atomic_or", 1,<br>
+ ((glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) |<br>
+ IMAGE_FUNCTION_HAS_RETURN));<br>
+<br>
+ add_image_function(glsl ? "imageAtomicXor" : "__intrinsic_image_atomic_xor",<br>
+ "__intrinsic_image_atomic_xor", 1,<br>
+ ((glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) |<br>
+ IMAGE_FUNCTION_HAS_RETURN));<br>
+<br>
+ add_image_function((glsl ? "imageAtomicExchange" :<br>
+ "__intrinsic_image_atomic_exchange"),<br>
+ "__intrinsic_image_atomic_exchange", 1,<br>
+ ((glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) |<br>
+ IMAGE_FUNCTION_HAS_RETURN));<br>
+<br>
+ add_image_function((glsl ? "imageAtomicCompSwap" :<br>
+ "__intrinsic_image_atomic_comp_swap"),<br>
+ "__intrinsic_image_atomic_comp_swap", 2,<br>
+ ((glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) |<br>
+ IMAGE_FUNCTION_HAS_RETURN));<br></blockquote><div><br>This is definitely an improvement, thanks. I still think there are some
minor tweaks that would reduce duplication of information, though:<br><br></div><div>- Precompute the value (glsl ? IMAGE_FUNCTION_EMIT_STUB : 0) rather than duplicating it at each add_image_function call site. Alternatively, just make emit_stub a bool parameter to add_image_function rather than including it in flags.<br>
<br></div><div>- Rather than repeating the pattern add_image_function((glsl ? BUILTIN_NAME : INTRINSIC_NAME), INTRINSIC_NAME, ...) everywhere, make BUILTIN_NAME and INTRINSIC_NAME the first two arguments to add_image_function(), and have add_image_function() pick which name to use based on emit_stub.<br>
<br></div><div>- Since IMAGE_FUNCTION_HAS_RETURN is by far the more common case, invert the sense of the flag (e.g. change it to IMAGE_FUNCTION_RETURNS_VOID).<br></div><div><br></div><div>With those three changes, the code would look like this:<br>
<br> const unsigned base_flags = glsl ? IMAGE_FUNCTION_EMIT_STUB : 0;<br><br> add_image_function("imageLoad", "__intrinsic_image_load", 0,<br> (base_flags |<br> IMAGE_FUNCTION_HAS_VECTOR_DATA_TYPE |<br>
IMAGE_FUNCTION_SUPPORTS_FLOAT_DATA_TYPE |<br> IMAGE_FUNCTION_READ_ONLY));<br><br> add_image_function("imageStore", "__intrinsic_image_store", 1,<br> (base_flags |<br>
IMAGE_FUNCTION_RETURNS_VOID |<br> IMAGE_FUNCTION_HAS_VECTOR_DATA_TYPE |<br> IMAGE_FUNCTION_SUPPORTS_FLOAT_DATA_TYPE |<br> IMAGE_FUNCTION_WRITE_ONLY));<br>
<br> add_image_function("imageAtomicAdd", "__intrinsic_image_atomic_add", 1,<br> base_flags);<br><br> add_image_function("imageAtomicMin", "__intrinsic_image_atomic_min", 1,<br>
base_flags);<br><br> add_image_function("imageAtomicMax", "__intrinsic_image_atomic_max", 1,<br> base_flags);<br><br> add_image_function("imageAtomicAnd", "__intrinsic_image_atomic_and", 1,<br>
base_flags);<br><br> add_image_function("imageAtomicOr", "__intrinsic_image_atomic_or", 1,<br> base_flags);<br><br> add_image_function("imageAtomicXor", "__intrinsic_image_atomic_xor", 1,<br>
base_flags);<br><br> add_image_function("imageAtomicExchange",<br> "__intrinsic_image_atomic_exchange", 1,<br> base_flags);<br><br> add_image_function("imageAtomicCompSwap",<br>
"__intrinsic_image_atomic_comp_swap", 2,<br> base_flags);<br><br></div><div>which IMHO is a lot more compact and easy to read.<br></div><div><br></div><div>With those changes, the patch is:<br>
<br>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br><br></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">
<div class="im">+}<br>
+<br>
ir_variable *<br>
builtin_builder::in_var(const glsl_type *type, const char *name)<br>
{<br>
</div>@@ -3991,6 +4165,99 @@ builtin_builder::_atomic_op(const char *intrinsic,<br>
return sig;<br>
}<br>
<br>
+ir_function_signature *<br>
+builtin_builder::_image_prototype(const glsl_type *image_type,<br>
+ const char *intrinsic_name,<br>
+ unsigned num_arguments,<br>
+ unsigned flags)<br>
+{<br>
+ const glsl_type *data_type = glsl_type::get_instance(<br>
+ image_type->fields.image.type,<br>
+ (flags & IMAGE_FUNCTION_HAS_VECTOR_DATA_TYPE ? 4 : 1),<br>
+ 1);<br>
+ const glsl_type *ret_type = (flags & IMAGE_FUNCTION_HAS_RETURN ? data_type :<br>
<div class="im">+ glsl_type::void_type);<br>
+<br>
+ /* Addressing arguments that are always present. */<br>
</div>+ ir_variable *image = in_var(image_type, "image");<br>
+ ir_variable *coord = in_var(<br>
<div class="im">+ glsl_type::ivec(image_type->coordinate_components()), "coord");<br>
+<br>
</div>+ ir_function_signature *sig = new_sig(<br>
+ ret_type, shader_image_load_store, 2, image, coord);<br>
<div class="im">+<br>
+ /* Sample index for multisample images. */<br>
+ if (image_type->fields.image.dimension == GLSL_IMAGE_DIM_MS)<br>
</div>+ sig->parameters.push_tail(in_var(glsl_type::int_type, "sample"));<br>
<div class="im">+<br>
+ /* Data arguments. */<br>
</div>+ for (unsigned i = 0; i < num_arguments; ++i)<br>
+ sig->parameters.push_tail(in_var(data_type,<br>
<div class="im">+ ralloc_asprintf(NULL, "arg%d", i)));<br>
+<br>
+ /* Set the maximal set of qualifiers allowed for this image<br>
+ * built-in. Function calls with arguments having fewer<br>
+ * qualifiers than present in the prototype are allowed by the<br>
+ * spec, but not with more, i.e. this will make the compiler<br>
+ * accept everything that needs to be accepted, and reject cases<br>
+ * like loads from write-only or stores to read-only images.<br>
+ */<br>
</div>+ image->image.read_only = flags & IMAGE_FUNCTION_READ_ONLY;<br>
+ image->image.write_only = flags & IMAGE_FUNCTION_WRITE_ONLY;<br>
<div class="im">+ image->image.coherent = true;<br>
+ image->image._volatile = true;<br>
+ image->image._restrict = true;<br>
+<br>
+ return sig;<br>
+}<br>
+<br>
</div>+ir_function_signature *<br>
+builtin_builder::_image(const glsl_type *image_type,<br>
+ const char *intrinsic_name,<br>
+ unsigned num_arguments,<br>
+ unsigned flags)<br>
+{<br>
+ ir_function_signature *sig = _image_prototype(image_type, intrinsic_name,<br>
+ num_arguments, flags);<br>
+<br>
+ if (flags & IMAGE_FUNCTION_EMIT_STUB) {<br>
+ ir_factory body(&sig->body, mem_ctx);<br>
+ ir_function *f = shader->symbols->get_function(intrinsic_name);<br>
+<br>
+ if (flags & IMAGE_FUNCTION_HAS_RETURN) {<br>
<div class="im">+ ir_variable *ret_val =<br>
+ body.make_temp(sig->return_type, "_ret_val");<br>
</div>+ body.emit(call(f, ret_val, sig->parameters));<br>
+ body.emit(ret(ret_val));<br>
+ } else {<br>
+ body.emit(call(f, NULL, sig->parameters));<br>
<div class="im">+ }<br>
+<br>
+ sig->is_defined = true;<br>
+<br>
+ } else {<br>
+ sig->is_intrinsic = true;<br>
+ }<br>
+<br>
+ return sig;<br>
+}<br>
+<br>
</div><div class="im">+ir_function_signature *<br>
+builtin_builder::_memory_barrier_intrinsic(builtin_available_predicate avail)<br>
+{<br>
+ MAKE_INTRINSIC(glsl_type::void_type, avail, 0);<br>
</div><div class="im">+ return sig;<br>
+}<br>
+<br>
</div><div class="im">+ir_function_signature *<br>
+builtin_builder::_memory_barrier(builtin_available_predicate avail)<br>
+{<br>
+ MAKE_SIG(glsl_type::void_type, avail, 0);<br>
+ body.emit(call(shader->symbols->get_function("__intrinsic_memory_barrier"),<br>
+ NULL, sig->parameters));<br>
</div><div class="im">+ return sig;<br>
+}<br>
+<br>
</div><div class="im"> /** @} */<br>
<br>
/******************************************************************************/<br>
--<br>
</div>1.8.5.1<br>
<br>
</blockquote></div><br></div></div>