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

[Iago Toral]

On Thu, 2016-08-18 at 14:25 +0300, Tapani Pälli wrote:
> 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).

Ah right, I did not notice that. Looks good to me then:

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

Iago

> > 
> > > 
> > >            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);
> > >            }
> > >         }
> > >   
> 
>