[Mesa-dev] [PATCH 05/11] nir: Separate dead write removal into its own pass

[Caio Marcelo de Oliveira Filho]

Hi,

> > +{
> > +   bool progress = false;
> > +
> > +   /* Find writes that are unused and can be removed. */
> > +   util_dynarray_foreach_reverse(unused_writes, struct write_entry,
> > entry) {
> > +      nir_deref_compare_result comp = nir_compare_derefs(dst, entry->dst);
> > +      if (comp & nir_derefs_a_contains_b_bit) {
> >
> 
> Mind throwing an assert in here:
> 
> assert((comp & nir_derefs_equal_bit) || mask == ~(nir_component_mask_t)0);

We can assert that.  We can have an entry for a copy between arrays a
and b, and see a store a[1].x that will invalidate the 'x' component
of the copy.

(...)

> > +      case nir_intrinsic_copy_deref: {
> > +         nir_deref_instr *src = nir_src_as_deref(intrin->src[1]);
> > +         nir_deref_instr *dst = nir_src_as_deref(intrin->src[0]);
> > +
> > +         /* Self-copy is removed. */
> > +         if (nir_compare_derefs(src, dst) & nir_derefs_equal_bit) {
> > +            nir_instr_remove(instr);
> > +            progress = true;
> > +            break;
> > +         }
> > +
> > +         uintptr_t mask = ~(1 << NIR_MAX_VEC_COMPONENTS);
> >
> 
> I don't think this does quite what you want.  Perhaps
> 
> nir_component_mask_t mask = ~(nir_component_mask_t)0;

I'm going with

nir_component_mask_t mask = (1 << glsl_get_vector_elements(dst->type)) - 1;


The idea is that we only fill bits that are valid, so we can detect
the condition that no bits are set and remove the entry.  Sounds good?


> 
> All of the comments were fairly trivial and nit-picky.  Assuming you're ok
> with the changes,
> 
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>



Caio