Hi,

> > +   }
> > +
> > +   if (new_written) {
> > +      /* Merge new information to the parent control flow node. */
> > +      if (written) {
> > +         written->modes |= new_written->modes;
> > +         struct hash_entry *ht_entry;
> > +         hash_table_foreach(new_written->derefs, ht_entry) {
> > +            _mesa_hash_table_insert_pre_hashed(written->derefs,
> > ht_entry->hash,
> > +                                               ht_entry->key,
> > ht_entry->data);
> >
> 
> Do you want to somehow OR masks together?  This is just picking one of the
> two masks.

You are correct.  Fixed.

Turns out the way the local code we are reusing here is structured, we
don't take much advantage of the fine-grained tracking here.  Added a
TODO about this.




> >  static void
> > -copy_entry_remove(struct copy_prop_var_state *state, struct copy_entry
> > *entry)
> > +copy_entry_remove(struct util_dynarray *copies,
> > +                  struct copy_entry *entry)
> >  {
> > -   list_del(&entry->link);
> > -   list_add(&entry->link, &state->copy_free_list);
> > +   *entry = util_dynarray_pop(copies, struct copy_entry);
> >
> 
> It might be worth a quick comment to justify that this works.  It took me a
> minute to figure out that you were re-ordering the array in the process.

Added a function comment describing what this does and stating it is
safe to use during a reverse iteration.  And also added a comment
highlighting how this works when it is the last element.

(...)

> 
> > +lookup_entry_and_kill_aliases(struct util_dynarray *copies,
> > +                              nir_deref_instr *deref,
> > +                              unsigned write_mask)
> >  {
> >     struct copy_entry *entry = NULL;
> > -   list_for_each_entry_safe(struct copy_entry, iter, &state->copies,
> > link) {
> > +   util_dynarray_foreach_reverse(copies, struct copy_entry, iter) {
> >
> 
> Also might be worth commenting why it's safe to remove elements while
> walking the array.

I think the comments to the copy_entry_remove suffice, but can add it
here before landing if you prefer.

The latest code is in

    https://gitlab.freedesktop.org/cmarcelo/mesa/commits/copy-prop

and all the issues I haven't commented are supposed to be fixed
according your comment (added new version to each patch that changed).
Merged the use patches into a single one

    intel/nir, freedreno/ir3: Use the separated dead write vars pass

Patches that still need R-b:

   nir: Copy propagation between blocks
   nir: Separate dead write removal into its own pass


Caio
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to