[Mesa-dev] [PATCH 1/4] glsl: Only set ir_variable::constant_value for const-decorated variables

[Ian Romanick]

On 10/08/2015 02:50 AM, Iago Toral wrote:
> On Wed, 2015-10-07 at 14:34 -0700, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> Right now we're also setting for uniforms, and that doesn't seem to hurt
>> things.  The next patch will make general global variables in GLSL ES,
>> and those definitely should not have constant_value set!
> 
> I think this breaks uniforms initializers in desktop GLSL (which are
> allowed). See link_uniform_initializers.cpp, that code expects uniforms
> with constant_value set.

You are correct.  The run through that CI system that I started when I
sent the patches out confirms... all the uniform initializer tests fail.
 I'm not sure why the linker doesn't just use constant_initializer
instead.  I knew the linker needed this data (because I wrote that
part), but I was not expecting it to use constant_value.  I'll dig more.

> Verified with this quick test:
> 
> --- vertex shader:
> #version 330
> layout(location = 0) in vec3 inVertexPosition;
> uniform mat4 MVP;
> uniform float test = 1.0;
> out float v2f;
> 
> void main()
> {
>    gl_Position = MVP * vec4(inVertexPosition, 1);
>    v2f = test;
> }
> 
> --- fragment shader:
> #version 330
> in float v2f;
> out vec3 color;
> 
> void main()
> {
>    color = vec3(v2f);
> }
> 
> Without your patch, the constant initialization of 'test' is handled by
> linker::set_uniform_initializer, which won't happen with this patch
> applied.
> 
> Iago
> 
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>  src/glsl/ast_to_hir.cpp | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> index 9511440..e3d4c44 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -3238,17 +3238,20 @@ process_initializer(ir_variable *var, ast_declaration *decl,
>>                                  decl->identifier);
>>                 if (var->type->is_numeric()) {
>>                    /* Reduce cascading errors. */
>> -                  var->constant_value = ir_constant::zero(state, var->type);
>> +                  var->constant_value = type->qualifier.flags.q.constant
>> +                     ? ir_constant::zero(state, var->type) : NULL;
>>                 }
>>              }
>>           } else {
>>              rhs = constant_value;
>> -            var->constant_value = constant_value;
>> +            var->constant_value = type->qualifier.flags.q.constant
>> +               ? constant_value : NULL;
>>           }
>>        } else {
>>           if (var->type->is_numeric()) {
>>              /* Reduce cascading errors. */
>> -            var->constant_value = ir_constant::zero(state, var->type);
>> +            var->constant_value = type->qualifier.flags.q.constant
>> +               ? ir_constant::zero(state, var->type) : NULL;
>>           }
>>        }
>>     }
> 
>