[Mesa-dev] [PATCH 3/5] glsl: Implement the required built-in functions when OES_shader_image_atomic is enabled.

[Francisco Jerez]

Ilia Mirkin <imirkin at alum.mit.edu> writes:

> On Mon, Feb 22, 2016 at 9:40 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>> This is basically just the same atomic functions exposed by
>> ARB_shader_image_load_store, with one exception:
>>
>>     "highp float imageAtomicExchange(
>>          coherent IMAGE_PARAMS,
>>          float data);"
>>
>> There's no float atomic exchange overload in the original
>> ARB_shader_image_load_store or GL 4.2, so this seems like new
>> functionality that requires specific back-end support and a separate
>> availability condition in the built-in function generator.
>> ---
>>  src/compiler/glsl/builtin_functions.cpp | 23 +++++++++++++++++++----
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/compiler/glsl/builtin_functions.cpp b/src/compiler/glsl/builtin_functions.cpp
>> index f488434..db74672 100644
>> --- a/src/compiler/glsl/builtin_functions.cpp
>> +++ b/src/compiler/glsl/builtin_functions.cpp
>> @@ -448,8 +448,16 @@ shader_image_load_store(const _mesa_glsl_parse_state *state)
>>  static bool
>>  shader_image_atomic(const _mesa_glsl_parse_state *state)
>>  {
>> -   return (state->is_version(420, 0) ||
>> -           state->ARB_shader_image_load_store_enable);
>> +   return (state->is_version(420, 320) ||
>> +           state->ARB_shader_image_load_store_enable ||
>> +           state->OES_shader_image_atomic_enable);
>> +}
>> +
>> +static bool
>> +shader_image_atomic_exchange_float(const _mesa_glsl_parse_state *state)
>> +{
>> +   return (state->is_version(450, 320) ||
>> +           state->OES_shader_image_atomic_enable);
>>  }
>>
>>  static bool
>> @@ -586,6 +594,7 @@ private:
>>        IMAGE_FUNCTION_WRITE_ONLY = (1 << 5),
>>        IMAGE_FUNCTION_AVAIL_ATOMIC = (1 << 6),
>>        IMAGE_FUNCTION_MS_ONLY = (1 << 7),
>> +      IMAGE_FUNCTION_AVAIL_ATOMIC_EXCHANGE = (1 << 8)
>>     };
>>
>>     /**
>> @@ -2981,7 +2990,9 @@ builtin_builder::add_image_functions(bool glsl)
>>     add_image_function((glsl ? "imageAtomicExchange" :
>>                         "__intrinsic_image_atomic_exchange"),
>>                        "__intrinsic_image_atomic_exchange",
>> -                      &builtin_builder::_image_prototype, 1, atom_flags);
>> +                      &builtin_builder::_image_prototype, 1,
>> +                      (flags | IMAGE_FUNCTION_AVAIL_ATOMIC_EXCHANGE |
>> +                       IMAGE_FUNCTION_SUPPORTS_FLOAT_DATA_TYPE));
>>
>>     add_image_function((glsl ? "imageAtomicCompSwap" :
>>                         "__intrinsic_image_atomic_comp_swap"),
>> @@ -5250,7 +5261,11 @@ builtin_builder::_image_prototype(const glsl_type *image_type,
>>        glsl_type::ivec(image_type->coordinate_components()), "coord");
>>
>>     const builtin_available_predicate avail =
>> -      (flags & IMAGE_FUNCTION_AVAIL_ATOMIC ? shader_image_atomic :
>> +      (((flags & IMAGE_FUNCTION_AVAIL_ATOMIC_EXCHANGE) &&
>> +        image_type->sampled_type == GLSL_TYPE_FLOAT) ?
>> +       shader_image_atomic_exchange_float :
>> +       flags & (IMAGE_FUNCTION_AVAIL_ATOMIC_EXCHANGE |
>> +                IMAGE_FUNCTION_AVAIL_ATOMIC) ? shader_image_atomic :
>>         shader_image_load_store);
>
> Wow, this is a monster expression. I know how much you like "pure"
> const values, but perhaps this might be more readable as a bunch of
> if's? I think you can actually do
>
> const foo; // no init
> if () foo = a
> else foo = b
>
> type of stuff. Your call though.
>
Yeah, you're right the expression was rather hard to read, but I was
thinking this is turning into kind of a monster function anyway?  How
about I move the calculation of the availability predicate into a
separate function?  v2 coming up in a minute.

> For some reason in my version I had decided to add a separate
> add_image_float_functions, but in hindsight that may have been
> misguided, as this seems to be sufficient.
>
>>     ir_function_signature *sig = new_sig(ret_type, avail, 2, image, coord);
>>
>> --
>> 2.7.0
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160222/ce093057/attachment.sig>