[Mesa-dev] [PATCH v3 2/5] glsl: add support for the imageSize builtin
Martin Peres
martin.peres at linux.intel.com
Wed Aug 19 07:01:48 PDT 2015
On 19/08/15 16:36, Francisco Jerez wrote:
> Martin Peres <martin.peres at linux.intel.com> writes:
>
>> The code is heavily inspired from Francisco Jerez's code supporting the
>> image_load_store extension.
>>
>> Backends willing to support this builtin should handle
>> __intrinsic_image_size.
>>
>> v2: Based on the review of Ilia Mirkin
>> - Enable the extension for GLES 3.1
>> - Fix indentation
>> - Fix the return type (float to int, number of components for CubeImages)
>> - Add a warning related to GLES 3.1
>>
>> v3: Based on the review of Francisco Jerez
>> - Refactor the code to share both add_image_function and _image with the other
>> image-related functions
>>
>> Signed-off-by: Martin Peres <martin.peres at linux.intel.com>
>> ---
>> src/glsl/builtin_functions.cpp | 109 +++++++++++++++++++++++++++++++++++------
>> 1 file changed, 93 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/glsl/builtin_functions.cpp b/src/glsl/builtin_functions.cpp
>> index 2175c66..5d0a825 100644
>> --- a/src/glsl/builtin_functions.cpp
>> +++ b/src/glsl/builtin_functions.cpp
>> @@ -399,6 +399,13 @@ shader_image_load_store(const _mesa_glsl_parse_state *state)
>> }
>>
>> static bool
>> +shader_image_size(const _mesa_glsl_parse_state *state)
>> +{
>> + return (state->is_version(430, 310) ||
>> + state->ARB_shader_image_size_enable);
>> +}
>> +
>> +static bool
>> gs_streams(const _mesa_glsl_parse_state *state)
>> {
>> return gpu_shader5(state) && gs_only(state);
>> @@ -492,6 +499,11 @@ private:
>> /** Create a new function and add the given signatures. */
>> void add_function(const char *name, ...);
>>
>> + typedef ir_function_signature *(builtin_builder::*image_prototype_ctr)(const glsl_type *image_type,
>> + const char *intrinsic_name,
>> + unsigned num_arguments,
>> + unsigned flags);
>> +
>> enum image_function_flags {
>> IMAGE_FUNCTION_EMIT_STUB = (1 << 0),
>> IMAGE_FUNCTION_RETURNS_VOID = (1 << 1),
>> @@ -507,6 +519,7 @@ private:
>> */
>> void add_image_function(const char *name,
>> const char *intrinsic_name,
>> + image_prototype_ctr prototype,
>> unsigned num_arguments,
>> unsigned flags);
>>
>> @@ -708,7 +721,12 @@ private:
>> const char *intrinsic_name,
>> unsigned num_arguments,
>> unsigned flags);
>> - ir_function_signature *_image(const glsl_type *image_type,
>> + ir_function_signature *_image_size_prototype(const glsl_type *image_type,
>> + const char *intrinsic_name,
>> + unsigned num_arguments,
>> + unsigned flags);
> The indentation looks weird. Mixing tabs and spaces?
No, I just forgot to re-align after changing the name. Sorry. It is fixed.
>
>> + ir_function_signature *_image(image_prototype_ctr prototype,
>> + const glsl_type *image_type,
>> const char *intrinsic_name,
>> unsigned num_arguments,
>> unsigned flags);
>> @@ -2552,6 +2570,7 @@ builtin_builder::add_function(const char *name, ...)
>> void
>> builtin_builder::add_image_function(const char *name,
>> const char *intrinsic_name,
>> + image_prototype_ctr prototype,
>> unsigned num_arguments,
>> unsigned flags)
>> {
>> @@ -2590,12 +2609,13 @@ builtin_builder::add_image_function(const char *name,
>> glsl_type::uimage2DMS_type,
>> glsl_type::uimage2DMSArray_type
>> };
>> +
>> ir_function *f = new(mem_ctx) ir_function(name);
>>
>> for (unsigned i = 0; i < ARRAY_SIZE(types); ++i) {
>> if (types[i]->sampler_type != GLSL_TYPE_FLOAT ||
>> (flags & IMAGE_FUNCTION_SUPPORTS_FLOAT_DATA_TYPE))
>> - f->add_signature(_image(types[i], intrinsic_name,
>> + f->add_signature(_image(prototype, types[i], intrinsic_name,
>> num_arguments, flags));
>> }
>>
>> @@ -2608,43 +2628,57 @@ builtin_builder::add_image_functions(bool glsl)
>> const unsigned flags = (glsl ? IMAGE_FUNCTION_EMIT_STUB : 0);
>>
>> add_image_function(glsl ? "imageLoad" : "__intrinsic_image_load",
>> - "__intrinsic_image_load", 0,
>> - (flags | IMAGE_FUNCTION_HAS_VECTOR_DATA_TYPE |
>> + "__intrinsic_image_load",
>> + &builtin_builder::_image_prototype, 0,
>> + (flags | IMAGE_FUNCTION_HAS_VECTOR_DATA_TYPE |
>> IMAGE_FUNCTION_SUPPORTS_FLOAT_DATA_TYPE |
>> IMAGE_FUNCTION_READ_ONLY));
>>
>> add_image_function(glsl ? "imageStore" : "__intrinsic_image_store",
>> - "__intrinsic_image_store", 1,
>> + "__intrinsic_image_store",
>> + &builtin_builder::_image_prototype, 1,
>> (flags | IMAGE_FUNCTION_RETURNS_VOID |
>> IMAGE_FUNCTION_HAS_VECTOR_DATA_TYPE |
>> IMAGE_FUNCTION_SUPPORTS_FLOAT_DATA_TYPE |
>> IMAGE_FUNCTION_WRITE_ONLY));
>>
>> add_image_function(glsl ? "imageAtomicAdd" : "__intrinsic_image_atomic_add",
>> - "__intrinsic_image_atomic_add", 1, flags);
>> + "__intrinsic_image_atomic_add",
>> + &builtin_builder::_image_prototype, 1, flags);
>>
>> add_image_function(glsl ? "imageAtomicMin" : "__intrinsic_image_atomic_min",
>> - "__intrinsic_image_atomic_min", 1, flags);
>> + "__intrinsic_image_atomic_min",
>> + &builtin_builder::_image_prototype, 1, flags);
>>
>> add_image_function(glsl ? "imageAtomicMax" : "__intrinsic_image_atomic_max",
>> - "__intrinsic_image_atomic_max", 1, flags);
>> + "__intrinsic_image_atomic_max",
>> + &builtin_builder::_image_prototype, 1, flags);
>>
>> add_image_function(glsl ? "imageAtomicAnd" : "__intrinsic_image_atomic_and",
>> - "__intrinsic_image_atomic_and", 1, flags);
>> + "__intrinsic_image_atomic_and",
>> + &builtin_builder::_image_prototype, 1, flags);
>>
>> add_image_function(glsl ? "imageAtomicOr" : "__intrinsic_image_atomic_or",
>> - "__intrinsic_image_atomic_or", 1, flags);
>> + "__intrinsic_image_atomic_or",
>> + &builtin_builder::_image_prototype, 1, flags);
>>
>> add_image_function(glsl ? "imageAtomicXor" : "__intrinsic_image_atomic_xor",
>> - "__intrinsic_image_atomic_xor", 1, flags);
>> + "__intrinsic_image_atomic_xor",
>> + &builtin_builder::_image_prototype, 1, flags);
>>
>> add_image_function((glsl ? "imageAtomicExchange" :
>> "__intrinsic_image_atomic_exchange"),
>> - "__intrinsic_image_atomic_exchange", 1, flags);
>> + "__intrinsic_image_atomic_exchange",
>> + &builtin_builder::_image_prototype, 1, flags);
>>
>> add_image_function((glsl ? "imageAtomicCompSwap" :
>> "__intrinsic_image_atomic_comp_swap"),
>> - "__intrinsic_image_atomic_comp_swap", 2, flags);
>> + "__intrinsic_image_atomic_comp_swap",
>> + &builtin_builder::_image_prototype, 2, flags);
>> +
>> + add_image_function(glsl ? "imageSize" : "__intrinsic_image_size",
>> + "__intrinsic_image_size",
>> + &builtin_builder::_image_size_prototype, 1, flags);
>> }
>>
>> ir_variable *
>> @@ -4818,13 +4852,56 @@ builtin_builder::_image_prototype(const glsl_type *image_type,
>> }
>>
>> ir_function_signature *
>> -builtin_builder::_image(const glsl_type *image_type,
>> +builtin_builder::_image_size_prototype(const glsl_type *image_type,
>> + const char *intrinsic_name,
>> + unsigned num_arguments,
>> + unsigned flags)
>> +{
>> + const glsl_type *ret_type;
>> + unsigned num_components = image_type->coordinate_components();
>> +
>> + /* From the ARB_shader_image_size extension:
>> + * "Cube images return the dimensions of one face."
>> + */
>> + if (image_type == glsl_type::imageCube_type ||
>> + image_type == glsl_type::iimageCube_type ||
>> + image_type == glsl_type::uimageCube_type) {
> If you like you could do (image_type->sampler_dimensionality ==
> GLSL_SAMPLER_DIM_CUBE && !image_type->sampler_array) here to avoid
> enumerating all image data types.
>
>> + num_components = 2;
>> + }
>> +
>> + /* FIXME: Add the highp precision qualified for GLES 3.10 when it is
>> + * becomes supported by mesa.
>> + */
> s/qualified/qualifier/ s/becomes //?
>
>> + ret_type = glsl_type::get_instance(GLSL_TYPE_INT, num_components, 1);
>> +
>> + ir_variable *image = in_var(image_type, "image");
>> + ir_function_signature *sig = new_sig(ret_type, shader_image_size, 1, image);
>> +
>> + /* 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->data.image_read_only = true;
>> + image->data.image_write_only = true;
>> + image->data.image_coherent = true;
>> + image->data.image_volatile = true;
>> + image->data.image_restrict = true;
>> +
>> + return sig;
>> +}
>> +
>> +ir_function_signature *
>> +builtin_builder::_image(image_prototype_ctr prototype,
>> + const glsl_type *image_type,
>> const char *intrinsic_name,
>> unsigned num_arguments,
>> unsigned flags)
>> {
>> - ir_function_signature *sig = _image_prototype(image_type, intrinsic_name,
>> - num_arguments, flags);
>> + ir_function_signature *sig = (this->*prototype)(image_type, intrinsic_name,
>> + num_arguments, flags);
> Weird indentation? With my nit-picks fixed this patch is:
Yeah, I again forgot to re-indent.
>
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
Thanks for the review! I will commit it shortly!
>
>>
>> if (flags & IMAGE_FUNCTION_EMIT_STUB) {
>> ir_factory body(&sig->body, mem_ctx);
>> --
>> 2.5.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list