On Tue, Jun 8, 2021 at 1:02 PM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Tue, Jun 8, 2021 at 11:31 AM Kewen.Lin <li...@linux.ibm.com> wrote:
> >
> > on 2021/6/7 下午10:46, Richard Biener wrote:
> > > On Wed, Jun 2, 2021 at 11:29 AM Kewen.Lin <li...@linux.ibm.com> wrote:
> > >>
> > >> Hi,
> > >>
> > >> As Richi suggested in PR100794, this patch is to remove
> > >> some unnecessary update_ssa calls with flag
> > >> TODO_update_ssa_only_virtuals, also do some refactoring.
> > >>
> > >> Bootstrapped/regtested on powerpc64le-linux-gnu P9,
> > >> x86_64-redhat-linux and aarch64-linux-gnu, built well
> > >> on Power9 ppc64le with --with-build-config=bootstrap-O3,
> > >> and passed both P8 and P9 SPEC2017 full build with
> > >> {-O3, -Ofast} + {,-funroll-loops}.
> > >>
> > >> Is it ok for trunk?
> > >
> > > LGTM, minor comment on the fancy C++:
> > >
> > > + auto cleanup = [&]() {
> > > + release_chains (chains);
> > > + free_data_refs (datarefs);
> > > + BITMAP_FREE (looparound_phis);
> > > + free_affine_expand_cache (&name_expansions);
> > > + };
> > >
> > > + cleanup ();
> > > + return 0;
> > >
> > > so that could have been
> > >
> > > class cleanup {
> > > ~cleanup()
> > > {
> > > release_chains (chains);
> > > free_data_refs (datarefs);
> > > BITMAP_FREE (looparound_phis);
> > > free_affine_expand_cache (&name_expansions);
> > > }
> > > } cleanup;
> > >
> > > ? Or some other means of adding registering a RAII-style cleanup?
> > > I mean, we can't wrap it all in
> > >
> > > try {...}
> > > finally {...}
> > >
> > > because C++ doesn't have finally.
> > >
> > > OK with this tiny part of the C++ refactoring delayed, but we can also
> > > simply
> > > discuss best options. At least for looparound_phis a good cleanup would
> > > be to pass the bitmap around and use auto_bitmap local to
> > > tree_predictive_commoning_loop ...
> > >
> >
> > Thanks Richi! One draft (not ready for review) is attached for the further
> > discussion. It follows the idea of RAII-style cleanup. I noticed that
> > Martin suggested stepping forward to make tree_predictive_commoning_loop
> > and its callees into one class (Thanks Martin), since there are not many
> > this kind of C++-style work functions, I want to double confirm which option
> > do you guys prefer?
> >
> > One point you might have seen is that to make tree_predictive_commoning_loop
> > and its callees as member functions of one class can avoid to pass bitmap
> > looparound_phis all around what's in the draft. :)
>
> Such general cleanup is of course desired - Giuliano started some of it within
> GSoC two years ago in the attempt to thread the compilation process. The
> cleanup then helps to get rid of global state which of course interferes here
> (and avoids unnecessary use of TLS vars).
>
> So yes, encapsulating global state into a class and making accessors
> member functions is something that is desired (but a lot of mechanical
> work).
Btw, the patch you posted is OK with me as well, it achieves the global
state removal, too.
Richard.
> Thanks
> Richard.
>
> > BR,
> > Kewen
> >
