[Mesa-dev] [PATCH v4 106/129] nir: Rework opt_copy_prop_vars to use deref instructions

[Kenneth Graunke]

On Friday, June 22, 2018 8:28:47 AM PDT Jason Ekstrand wrote:
> On Thu, Jun 21, 2018 at 1:03 AM, Kenneth Graunke <kenneth at whitecape.org>
> wrote:
> 
> > On Thursday, May 31, 2018 10:06:47 PM PDT Jason Ekstrand wrote:
> > [snip]
> > > @@ -529,57 +509,55 @@ static bool
> > >  load_from_deref_entry_value(struct copy_prop_var_state *state,
> > >                              struct copy_entry *entry,
> > >                              nir_builder *b, nir_intrinsic_instr *intrin,
> > > -                            nir_deref_var *src, struct value *value)
> > > +                            nir_deref_instr *src, struct value *value)
> > >  {
> > >     *value = entry->src;
> > >
> > > -   /* Walk the deref to get the two tails and also figure out if we
> > need to
> > > -    * specialize any wildcards.
> > > -    */
> > > -   bool need_to_specialize_wildcards = false;
> > > -   nir_deref *entry_tail = &entry->dst->deref;
> > > -   nir_deref *src_tail = &src->deref;
> > > -   while (entry_tail->child && src_tail->child) {
> > > -      assert(src_tail->child->deref_type ==
> > entry_tail->child->deref_type);
> > > -      if (src_tail->child->deref_type == nir_deref_type_array) {
> > > -         nir_deref_array *entry_arr = nir_deref_as_array(entry_tail-
> > >child);
> > > -         nir_deref_array *src_arr = nir_deref_as_array(src_tail->
> > child);
> > > -
> >
> > I think there might be a bug here...note this condition...
> >
> > > -         if (src_arr->deref_array_type != nir_deref_array_type_wildcard
> > &&
> > > -             entry_arr->deref_array_type ==
> > nir_deref_array_type_wildcard)
> >
> > Old: Source NOT wildcard, dest is wildcard.
> >
> > > -            need_to_specialize_wildcards = true;
> > > -      }
> > > +   b->cursor = nir_instr_remove(&intrin->instr);
> > >
> > > -      entry_tail = entry_tail->child;
> > > -      src_tail = src_tail->child;
> > > +   nir_deref_path entry_dst_path, src_path;
> > > +   nir_deref_path_init(&entry_dst_path, entry->dst, state->mem_ctx);
> > > +   nir_deref_path_init(&src_path, src, state->mem_ctx);
> > > +
> > > +   bool need_to_specialize_wildcards = false;
> > > +   nir_deref_instr **entry_p = &entry_dst_path.path[1];
> > > +   nir_deref_instr **src_p = &src_path.path[1];
> > > +   while (*entry_p && *src_p) {
> > > +      nir_deref_instr *entry_tail = *entry_p++;
> > > +      nir_deref_instr *src_tail = *src_p++;
> > > +
> > > +      if (src_tail->deref_type == nir_deref_type_array &&
> > > +          entry_tail->deref_type == nir_deref_type_array_wildcard)
> >
> > New: Source IS wildcard, dest is wildcard.  I think you want != on the
> > source condition to match the old behavior.
> >
> 
> I don't think there's a bug here.  With the old mechanism, we had
> deref_type_array and then within that deref_array_type_direct, _indirect,
> and _wildcard.  In the new scheme, we have deref_type_array and
> deref_type_array_wildcard.  So deref_type == nir_deref_type_array in the
> new translates to deref_type == nir_deref_type_array && deref_array_type !=
> nir_deref_array_type_wildcard.  Does that make sense?
> 

I missed that it also changed from != wildcard to == array. :(

Looks okay after all.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180622/9966d92e/attachment.sig>