Can you please re-send the latest version of this patch? It's easier to
comment on the ML.
On Mon, Oct 15, 2018 at 12:44 PM Caio Marcelo de Oliveira Filho <
caio.olive...@intel.com> wrote:
> 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
