[PATCH] predcom: Adjust some unnecessary update_ssa calls

Richard Biener richard.guenther@gmail.com
Tue Jun 8 11:09:57 GMT 2021


On Tue, Jun 8, 2021 at 1:02 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Tue, Jun 8, 2021 at 11:31 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
> >
> > on 2021/6/7 下午10:46, Richard Biener wrote:
> > > On Wed, Jun 2, 2021 at 11:29 AM Kewen.Lin <linkw@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
> >


More information about the Gcc-patches mailing list