[Mesa-dev] [PATCH] glsl: Allow any sort of sampler array indexing with GLSL ES < 3.00

[Ian Romanick]

On 04/07/2015 03:22 AM, Francisco Jerez wrote:
> Tapani Pälli <tapani.palli at intel.com> writes:
> 
>> From: Kalyan Kondapally <kalyan.kondapally at intel.com>
>>
>> Dynamic indexing of sampler arrays is prohibited by GLSL ES 3.00.
>> Earlier versions allow 'constant-index-expression' indexing, where
>> index can contain a loop induction variable.
>>
>> Patch allows dynamic indexing for sampler arrays when GLSL ES < 3.00.
>> This change makes 'sampler-array-index.frag' parser test in Piglit
>> pass + fishgl.com works when running Chrome on OpenGL ES 2.0 backend.
>>
>> v2: small change and some more commit message (Tapani)
>>
>> Signed-off-by: Kalyan Kondapally <kalyan.kondapally at intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84225
> 
> Looks good, but did you check what happens now if the shader uses actual
> variable indexing (i.e. which lowering cannot turn into a constant) on
> an implementation that doesn't support it?  Hopefully no crashes or
> hangs?

I think we should add a post-link check that no dynamic indexing remains
after all the optimizations are complete.  The intention if the ES2
language was to allow cases where the dynamic indexing could be
optimized away.  This was redacted in ES3 because each optimizer was
differently capable, so a shader that worked on one driver/GPU might
fail on another... even from the same vendor.

Adding the post-link check should prevent the problems the Curro
(rightly) worried about, and it should still allow the WebGL demo to work.

>> ---
>>  src/glsl/ast_array_index.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/glsl/ast_array_index.cpp b/src/glsl/ast_array_index.cpp
>> index ecef651..b2609b6 100644
>> --- a/src/glsl/ast_array_index.cpp
>> +++ b/src/glsl/ast_array_index.cpp
>> @@ -226,7 +226,7 @@ _mesa_ast_array_index_to_hir(void *mem_ctx,
>>         * dynamically uniform expression is undefined.
>>         */
>>        if (array->type->element_type()->is_sampler()) {
>> -	 if (!state->is_version(130, 100)) {
>> +	 if (!state->is_version(130, 300)) {
>>  	    if (state->es_shader) {
>>  	       _mesa_glsl_warning(&loc, state,
>>  				  "sampler arrays indexed with non-constant "

It looks like this is what e3ded7f should have made this code.

Looking at the rest of the surrounding code, I don't think this is quite
right... at the very least, it's not easy to follow.  You can blame me
and Paul for that.  I think this is correct and easier to follow:

   if (!state->is_version(400, 0) && !state->ARB_gpu_shader5_enable) {
      if (state->is_version(130, 300))
         _mesa_glsl_error(&loc, state,
                            "sampler arrays indexed with non-constant "
                            "expressions are forbidden in GLSL %s "
                            "and later"
                            state->es_shader ? "ES 3.00" : "1.30");
      else if (state->es_shader)
         _mesa_glsl_warning(&loc, state,
                            "sampler arrays indexed with non-constant "
                            "expressions are optional in %s and will "
                            "be forbidden in GLSL ES 3.00 and later"
                            state->version_string());
      else
         _mesa_glsl_warning(&loc, state,
                            "sampler arrays indexed with non-constant "
                            "expressions will be forbidden in GLSL "
                            "1.30 and later");
   }

>> -- 
>> 2.1.0
>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev