[Mesa-dev] [PATCH] glsl: fix key used for hashing switch statement cases

[Tapani Pälli]

On 08/18/2016 01:08 PM, Iago Toral wrote:
> On Thu, 2016-08-18 at 11:57 +0300, Tapani Pälli wrote:
>> Implementation previously used case value itself as the key, however
>> afterhash implementation change by ee02a5e we cannot use 0 as key.
>> Patch uses _mesa_hash_data to formulate a suitable key for this hash.
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97309
>> ---
>>   src/compiler/glsl/ast_to_hir.cpp | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>> b/src/compiler/glsl/ast_to_hir.cpp
>> index e03a6e3..53fc4d6 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -6180,9 +6180,11 @@ ast_case_label::hir(exec_list *instructions,
>>            /* Stuff a dummy value in to allow processing to continue.
>> */
>>            label_const = new(ctx) ir_constant(0);
>>         } else {
>> +         unsigned hash_key = _mesa_hash_data(&label_const-
>>> value.u[0],
>> +                                             sizeof(unsigned));
> Shouldn't this take (void *) label_const->value[0] instead? It is the
> value of the label we want to use as key, not the memory address where
> it is stored, right?

This works ok because of the way _mesa_hash_data is implemented. It 
takes address of some data and then for contents found from that address 
it calculates a hash, so it'll do that for the value of the label (see 
_mesa_fnv32_1a_accumulate_block).

>>            ast_expression *previous_label = (ast_expression *)
>>            hash_table_find(state->switch_state.labels_ht,
>> -                         (void *)(uintptr_t)label_const-
>>> value.u[0]);
>> +                         (void *)(uintptr_t)hash_key);
> Actually, you use the value here, so I guess the above was a mistake?
>
>>            if (previous_label) {
>>               YYLTYPE loc = this->test_value->get_location();
>> @@ -6193,7 +6195,7 @@ ast_case_label::hir(exec_list *instructions,
>>            } else {
>>               hash_table_insert(state->switch_state.labels_ht,
>>                                 this->test_value,
>> -                              (void *)(uintptr_t)label_const-
>>> value.u[0]);
>> +                              (void *)(uintptr_t)hash_key);
>>            }
>>         }
>>