[Mesa-dev] [PATCH 1/2] glsl: add support for textureSamples function

[Tapani Pälli]

On 08/12/2015 03:00 PM, Ilia Mirkin wrote:
> On Wed, Aug 12, 2015 at 2:34 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
>> Hi Ilia;
>>
>> Some comments below;
>>
>>
>> On 08/12/2015 04:51 AM, Ilia Mirkin wrote:
>>>
>>> ---
>>>    src/glsl/builtin_functions.cpp             | 32
>>> +++++++++++++++++++++++++++++-
>>>    src/glsl/glcpp/glcpp-parse.y               |  3 +++
>>>    src/glsl/glsl_parser_extras.cpp            |  1 +
>>>    src/glsl/glsl_parser_extras.h              |  2 ++
>>>    src/glsl/ir.cpp                            |  5 +++--
>>>    src/glsl/ir.h                              |  3 ++-
>>>    src/glsl/ir_clone.cpp                      |  1 +
>>>    src/glsl/ir_equals.cpp                     |  1 +
>>>    src/glsl/ir_hv_accept.cpp                  |  1 +
>>>    src/glsl/ir_print_visitor.cpp              |  6 ++++--
>>>    src/glsl/ir_reader.cpp                     |  6 +++++-
>>>    src/glsl/ir_rvalue_visitor.cpp             |  1 +
>>>    src/glsl/nir/glsl_to_nir.cpp               |  5 +++++
>>>    src/glsl/nir/nir.h                         |  4 +++-
>>>    src/glsl/nir/nir_print.c                   |  3 +++
>>>    src/glsl/opt_tree_grafting.cpp             |  1 +
>>>    src/mesa/main/mtypes.h                     |  1 +
>>>    src/mesa/state_tracker/st_glsl_to_tgsi.cpp |  2 ++
>>>    18 files changed, 70 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/glsl/builtin_functions.cpp
>>> b/src/glsl/builtin_functions.cpp
>>> index 2175c66..1a71d74 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_samples(const _mesa_glsl_parse_state *state)
>>> +{
>>> +   return state->is_version(450, 0) ||
>>> +      state->ARB_shader_texture_image_samples_enable;
>>
>>
>> According to the extension spec, 4.30 should be enough for this.
>
> The extension spec says what GLSL version it's based on, not what GLSL
> version is needed to have the functionality auto-enabled. imageSamples
> and textureSamples are part of GLSL 4.50, but the extension spec is
> written against GLSL 4.30.

Oh right, this is correct. Either 4.50 or the extension enabled.

>>
>>> +}
>>> +
>>> +static bool
>>>    gs_streams(const _mesa_glsl_parse_state *state)
>>>    {
>>>       return gpu_shader5(state) && gs_only(state);
>>> @@ -630,10 +637,10 @@ private:
>>>       B1(any);
>>>       B1(all);
>>>       B1(not);
>>> -   B2(textureSize);
>>
>>
>> unrelated cleanup?
>
> Yeah... actually that should probably all just become BA2(textureSize)
> or something. I guess I should do that separately.

ok

>
>>
>>
>>>       ir_function_signature *_textureSize(builtin_available_predicate
>>> avail,
>>>                                           const glsl_type *return_type,
>>>                                           const glsl_type *sampler_type);
>>> +   B1(textureSamples);
>>>
>>>    /** Flags to _texture() */
>>>    #define TEX_PROJECT 1
>>> @@ -1372,6 +1379,16 @@ builtin_builder::create_builtins()
>>>                    _textureSize(texture_multisample, glsl_type::ivec3_type,
>>> glsl_type::usampler2DMSArray_type),
>>>                    NULL);
>>>
>>> +   add_function("textureSamples",
>>> +                _textureSamples(glsl_type::sampler2DMS_type),
>>> +                _textureSamples(glsl_type::isampler2DMS_type),
>>> +                _textureSamples(glsl_type::usampler2DMS_type),
>>> +
>>> +                _textureSamples(glsl_type::sampler2DMSArray_type),
>>> +                _textureSamples(glsl_type::isampler2DMSArray_type),
>>> +                _textureSamples(glsl_type::usampler2DMSArray_type),
>>> +                NULL);
>>> +
>>>       add_function("texture",
>>>                    _texture(ir_tex, v130, glsl_type::vec4_type,
>>> glsl_type::sampler1D_type,  glsl_type::float_type),
>>>                    _texture(ir_tex, v130, glsl_type::ivec4_type,
>>> glsl_type::isampler1D_type, glsl_type::float_type),
>>> @@ -4079,6 +4096,19 @@
>>> builtin_builder::_textureSize(builtin_available_predicate avail,
>>>    }
>>>
>>>    ir_function_signature *
>>> +builtin_builder::_textureSamples(const glsl_type *sampler_type)
>>> +{
>>> +   ir_variable *s = in_var(sampler_type, "sampler");
>>> +   MAKE_SIG(glsl_type::int_type, shader_samples, 1, s);
>>> +
>>> +   ir_texture *tex = new(mem_ctx) ir_texture(ir_texture_samples);
>>> +   tex->set_sampler(new(mem_ctx) ir_dereference_variable(s),
>>> glsl_type::int_type);
>>> +   body.emit(ret(tex));
>>> +
>>> +   return sig;
>>> +}
>>> +
>>> +ir_function_signature *
>>>    builtin_builder::_texture(ir_texture_opcode opcode,
>>>                              builtin_available_predicate avail,
>>>                              const glsl_type *return_type,
>>> diff --git a/src/glsl/glcpp/glcpp-parse.y b/src/glsl/glcpp/glcpp-parse.y
>>> index dd5ec2a..52cae08 100644
>>> --- a/src/glsl/glcpp/glcpp-parse.y
>>> +++ b/src/glsl/glcpp/glcpp-parse.y
>>> @@ -2478,6 +2478,9 @@
>>> _glcpp_parser_handle_version_declaration(glcpp_parser_t *parser, intmax_t
>>> versio
>>>                if (extensions->ARB_shader_image_load_store)
>>>                   add_builtin_define(parser,
>>> "GL_ARB_shader_image_load_store", 1);
>>>
>>> +             if (extensions->ARB_shader_texture_image_samples)
>>> +                add_builtin_define(parser,
>>> "GL_ARB_shader_texture_image_samples", 1);
>>> +
>>
>>
>> Now that we have GL_ARB_shader_image_load_store, before exposing this
>> extension we should have imageSamples() supported too. Otherwise these
>> changes look good for me.
>
> I'm not exposing the extension... *if* the extension is exposed, I'm
> adding the define. But no driver sets that to true... yet.
>
> I was hoping to get feedback on this before doing the
> (trivial-for-i965) imageSamples impl. Also it would conflict trivially
> with some in-flight stuff from Martin.

This sounds fine. imageSamples() can be done as follow-up and only then 
enabling the extension. For this patch;

Reviewed-by: Tapani Pälli <tapani.palli at intel.com>