[Mesa-dev] [PATCH 2/2] glsl: fix 'shared' layout qualifier related regressions

[Samuel Iglesias Gonsálvez]

On 12/11/15 15:38, Timothy Arceri wrote:
> 
> 
> On 13 November 2015 12:22:40 am AEDT, "Samuel Iglesias Gonsálvez" <siglesias at igalia.com> wrote:
>> Commit 8b28b35 added 'shared' as a keyword for compute shaders
>> but it broke the existing 'shared' layout qualifier support for
>> uniform and shader storage blocks.
>>
>> This patch fixes 578 dEQP-GLES31.functional.ssbo.* tests.
>>
>> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>> ---
>> src/glsl/glsl_parser.yy | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
>> index 4ac8e45..592630e 100644
>> --- a/src/glsl/glsl_parser.yy
>> +++ b/src/glsl/glsl_parser.yy
>> @@ -1199,8 +1199,6 @@ layout_qualifier_id:
>>       if (!$$.flags.i && state->has_uniform_buffer_objects()) {
>>          if (match_layout_qualifier($1, "std140", state) == 0) {
>>             $$.flags.q.std140 = 1;
>> -         } else if (match_layout_qualifier($1, "shared", state) == 0)
>> {
>> -            $$.flags.q.shared = 1;
> 
> 
> I think you need to leave this here, as layout identifiers are case insensitive.
> 

Good catch. Also it is needed when CS is not supported.

> 
>>         } else if (match_layout_qualifier($1, "std430", state) == 0) {
>>             $$.flags.q.std430 = 1;
>>   } else if (match_layout_qualifier($1, "column_major", state) == 0) {
>> @@ -1618,6 +1616,15 @@ layout_qualifier_id:
>>                             "layout qualifier `%s' is used", $1);
>>       }
>>    }
>> +   | SHARED
>> +   {
>> +      memset(& $$, 0, sizeof($$));
>> +
>> +      if (!$$.flags.i && state->has_uniform_buffer_objects()) {
>> +	 $$.flags.q.shared = 1;
> 
> The right place for this is probably interface_block_layout_qualifier see the commet above it for the same scenario with row_major.
> 
> http://cgit.freedesktop.org/mesa/mesa/tree/src/glsl/glsl_parser.yy#n1635
> 

Right, I am going to send a v2.

>> +	 $$.flags.q.shared_storage = 0;
> 
> Are you sure this is needed? 
> 

After some testing, I verified it is not needed. I am going to remove it.

Thanks for the review!

Sam

>> +      }
>> +   }
>>    ;
>>
>> /* This is a separate language rule because we parse these as tokens
>> -- 
>> 2.5.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>