<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Oct 15, 2018 at 12:37 PM Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com">caio.oliveira@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
> > +{<br>
> > +   bool progress = false;<br>
> > +<br>
> > +   /* Find writes that are unused and can be removed. */<br>
> > +   util_dynarray_foreach_reverse(unused_writes, struct write_entry,<br>
> > entry) {<br>
> > +      nir_deref_compare_result comp = nir_compare_derefs(dst, entry->dst);<br>
> > +      if (comp & nir_derefs_a_contains_b_bit) {<br>
> ><br>
> <br>
> Mind throwing an assert in here:<br>
> <br>
> assert((comp & nir_derefs_equal_bit) || mask == ~(nir_component_mask_t)0);<br>
<br>
We can assert that.  We can have an entry for a copy between arrays a<br>
and b, and see a store a[1].x that will invalidate the 'x' component<br>
of the copy.<br></blockquote><div><br></div><div>Do you mean, "we can't assert that"?</div><div><br></div><div>I'm trying to think about whether or not the type of per-component invalidation you're talking about there is valid or not.  If we can assume that all struct copies are split and that all copies are fully qualified (i.e., they end in a vector or scalar with wildcards for all the arrays), then I think such inference is fine.  Maybe worth a comment that such is intentional?<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(...)<br>
<br>
> > +      case nir_intrinsic_copy_deref: {<br>
> > +         nir_deref_instr *src = nir_src_as_deref(intrin->src[1]);<br>
> > +         nir_deref_instr *dst = nir_src_as_deref(intrin->src[0]);<br>
> > +<br>
> > +         /* Self-copy is removed. */<br>
> > +         if (nir_compare_derefs(src, dst) & nir_derefs_equal_bit) {<br>
> > +            nir_instr_remove(instr);<br>
> > +            progress = true;<br>
> > +            break;<br>
> > +         }<br>
> > +<br>
> > +         uintptr_t mask = ~(1 << NIR_MAX_VEC_COMPONENTS);<br>
> ><br>
> <br>
> I don't think this does quite what you want.  Perhaps<br>
> <br>
> nir_component_mask_t mask = ~(nir_component_mask_t)0;<br>
<br>
I'm going with<br>
<br>
nir_component_mask_t mask = (1 << glsl_get_vector_elements(dst->type)) - 1;<br>
<br>
<br>
The idea is that we only fill bits that are valid, so we can detect<br>
the condition that no bits are set and remove the entry.  Sounds good?<br></blockquote><div><br></div><div>Seems reasonable.  Again, this assumes that dst-type is a vector or scalar and not a struct, array, or other odd type.</div><div><br></div><div>--Jason<br></div></div></div>