Re: [PATCH] Refactor tree-loop-distribution for thread safety

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Nov 12, 2019 at 2:31 PM Giuliano Belinassi
<giuliano.belinassi@usp.br> wrote:
>
> Hi, Richard.
>
> On 11/12, Richard Biener wrote:
> > On Sat, Nov 9, 2019 at 3:26 PM Giuliano Belinassi
> > <giuliano.belinassi@usp.br> wrote:
> > >
> > > Hi all,
> > >
> > > This patch refactors tree-loop-distribution.c for thread safety without
> > > use of C11 __thread feature. All global variables were moved to a struct
> > > which is initialized at ::execute time.
> >
> > Thanks for working on this.  I've been thinking on how to make this
> > nicer which naturally leads to the use of C++ classes and member
> > functions which get 'this' for free.  This means all functions that
> > make use of 'priv' in your patch would need to become member
> > functions of the class and pass_loop_distribution::execute would
> > wrap it like
>
> Wouldn't it require that we have one instance of the
> `pass_loop_distribution` class for each thread? I don't know how
> the pass manager would handle this at the current state, but probably
> I will need to patch it in my branch.

I don't mean to re-use pass_loop_distribution itself, just move all
functions as methods into a new separate class you instantiate
at pass_loop_distribution::execute time, so much like your patch
but use 'this' instead of the explicit passing of the structure pointer.

> >
> > unsigned int
> > pass_loop_distribution::execute (function *fun)
> > {
> >   return priv_pass_vars().execute (fun);
> > }
>
>
> >
> > please find a better name for 'priv_pass_vars' since you can't
> > reuse that name for other passes due to C++ ODR rules.
> > I would suggest 'loop_distribution'.
> >
> > Can you try if going this route works well?
>
> Of course :)
>
> >
> > Thanks,
> > Richard.
> >
> > > I can install this patch myself in trunk if it's OK.
> > >
> > > gcc/ChangeLog
> > > 2019-11-09  Giuliano Belinassi  <giuliano.belinassi@usp.br>
> > >
> > >         * cfgloop.c (get_loop_body_in_custom_order): New.
> > >         * cfgloop.h (get_loop_body_in_custom_order): New prototype.
> > >         * tree-loop-distribution.c (struct priv_pass_vars): New.
> > >         (bb_top_order_cmp_r): New.
> > >         (create_rdg_vertices): Update prototype.
> > >         (stmts_from_loop): Same as above.
> > >         (update_for_merge): Same as above.
> > >         (partition_merge_into): Same as above.
> > >         (get_data_dependence): Same as above.
> > >         (data_dep_in_cycle_p): Same as above.
> > >         (update_type_for_merge): Same as above.
> > >         (build_rdg_partition_for-vertex): Same as above.
> > >         (classify_builtin_ldst): Same as above.
> > >         (classify_partition): Same as above.
> > >         (share_memory_accesses): Same as above.
> > >         (rdg_build_partitions): Same as above.
> > >         (pg_add_dependence_edges): Same as above.
> > >         (build_partition_graph): Same as above.
> > >         (merge_dep_scc_partitions): Same as above.
> > >         (break_alias_scc_partitions): Same as above.
> > >         (finalize_partitions): Same as above.
> > >         (distribute_loop): Same as above.
> > >         (bb_top_order_init): New function.
> > >         (bb_top_order_destroy): New function.
> > >         (pass_loop_distribution::execute): Initialize struct priv.
> > >
> > > Thank you,
> > > Giuliano.
>
> Thank you,
> Giuliano.