Re: [PATCH] predcom: Adjust some unnecessary update_ssa calls

[Martin Sebor via Gcc-patches]

On 6/7/21 8:46 AM, Richard Biener via Gcc-patches 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 agree this would be better than invoking the cleanup lambda
explicitly.

Going a step further would be to encapsulate all the data in a class
(eliminating the static variables) and make
tree_predictive_commoning_loop() its member function (along with
whatever others it calls), and have the dtor take care of
the cleanup.

Of course, if the data types were classes with ctors and dtors
(including, for example, auto_vec rather than vec for chains)
the cleanup would just happen and none of the explicit calls
would be necessary.

Martin

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,
Richard.

BR,
Kewen
-----
gcc/ChangeLog:

         * tree-predcom.c (execute_pred_commoning): Remove update_ssa call.
         (tree_predictive_commoning_loop): Factor some cleanup stuffs into
         lambda function cleanup, remove scev_reset call, and adjust return
         value.
         (tree_predictive_commoning): Adjust for different changed values,
         only set flag TODO_update_ssa_only_virtuals if changed.
         (pass_data pass_data_predcom): Remove TODO_update_ssa_only_virtuals
         from todo_flags_finish.