[patch] Extend GIMPLE store merging to throwing stores

[Richard Biener]

On Fri, Jul 26, 2019 at 12:56 PM Eric Botcazou <ebotcazou@adacore.com> wrote:
>
> Hi,
>
> one of the effects of -fnon-call-exceptions is that the memory accesses are
> considered trapping by default, i.e. unless you can prove otherwise.  If, in
> addition to this, the code is covered by an exception handler, such memory
> accesses are the sources of an EH edge, which means that they end their basic
> block.  This thwarts a fair number of optimizations, especially the local ones
> among which a very badly affected example is GIMPLE store merging.
>
> So the attached patch adds (minimal) support for merging throwing stores when
> -fnon-call-exceptions is enabled and there are active exception handlers in
> the function.  The idea is straightforward (to record the index number of the
> landing pad and merge consecutive stores with the same number) so the meat of
> the patch is technicalities required to first streamline the CFG, then detect
> the candidates throwing stores and finally properly update the CFG at the end.
>
> We have real-world examples for which this helps a lot in Ada, typically when
> you're inlining an elaboration routine (constructor in Ada parlance) into a
> region covered by an exception handler.
>
> Tested on x86_64-suse-linux, OK for the mainline?

+/* Return the index number of the landing pad for STMT, if any.  */
+
+static int
+lp_nr_for_store (gimple *stmt)
+{
+  if (!cfun->can_throw_non_call_exceptions || !cfun->eh)
+    return 0;
+
+  if (!stmt_could_throw_p (cfun, stmt))
+    return 0;
+
+  return lookup_stmt_eh_lp (stmt);
+}

Did you add the wrapper as compile-time optimization?  That is,
I don't see why simply calling lookup_stmt_eh_lp wouldn't work?

+  /* If the function can throw and catch non-call exceptions, we'll be trying
+     to merge stores across different basic blocks so we need to first unsplit
+     the EH edges in order to streamline the CFG of the function.  */
+  if (cfun->can_throw_non_call_exceptions && cfun->eh)
+    {
+      free_dominance_info (CDI_DOMINATORS);
+      maybe_remove_unreachable_handlers ();
+      changed = unsplit_all_eh ();
+      if (changed)
+       delete_unreachable_blocks ();
+    }

uh, can unsplitting really result in unreachable blocks or does it
merely forget to delete forwarders it made unreachable?  Removing
unreachable handlers is also to make things match better?  Just
wondering how much of this work we could delay to the first
store-merging opportunity with EH we find (but I don't care too much
about -fnon-call-exceptions).

To isolate the details above maybe move this piece into a helper
in tree-eh.c so you also can avoid exporting unsplit_all_eh?

Otherwise looks OK to me.

Thanks,
Richard.

>
> 2019-07-26  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree-eh.h (unsplit_all_eh): Declare.
>         * tree-eh.c (maybe_remove_unreachable_handlers): Detect more cases.
>         (unsplit_all_eh): Make public.
>         * gimple-ssa-store-merging.c: Include cfganal.h cfgcleanup.h except.h.
>         (struct store_immediate_info): Add lp_nr field.
>         (store_immediate_info::store_immediate_info): Add NR2 parameter and
>         initialize lp_nr with it.
>         (struct merged_store_group): Add lp_nr and only_constants fields.
>         (merged_store_group::merged_store_group): Initialize them.
>         (merged_store_group::can_be_merged_into): Deal with them.
>         (pass_store_merging): Rename terminate_and_release_chain into
>         terminate_and_process_chain.
>         (pass_store_merging::terminate_and_process_all_chains): Adjust to above
>         renaming and remove useless assertions.
>         (pass_store_merging::terminate_all_aliasing_chains): Small tweak.
>         (stmts_may_clobber_ref_p): Be prepared for different basic blocks.
>         (imm_store_chain_info::coalesce_immediate_stores): Use only_constants
>         instead of always recomputing it and compare lp_nr.
>         (imm_store_chain_info::output_merged_store): If the group is in an
>         active EH region, register new stores if they can throw.  Moreover,
>         if the insertion has created new basic blocks, adjust the PHI nodes
>         of the post landing pad.
>         (imm_store_chain_info::output_merged_stores): If the original stores
>         are in an active EH region, deregister them.
>         (lhs_valid_for_store_merging_p): Prettify.
>         (adjust_bit_pos): New function extracted from...
>         (mem_valid_for_store_merging): ...here.  Use it for the base address
>         and also for the offset if it is the addition of a constant.
>         (lp_nr_for_store): New function.
>         (pass_store_merging::process_store): Change return type to bool.
>         Call lp_nr_for_store to initialize the store info.  Propagate the
>         return status of various called functions to the return value.
>         (store_valid_for_store_merging_p): New predicate.
>         (enum basic_block_status): New enumeration.
>         (get_status_for_store_merging): New function.
>         (pass_store_merging::execute): If the function can throw and catch
>         non-call exceptions, unsplit the EH edges on entry and clean up the
>         CFG on exit if something changed.  Call get_status_for_store_merging
>         for every basic block and keep the chains open across basic blocks
>         when possible.  Terminate and process open chains at the end, if any.
>
>
> 2019-07-26  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/opt81.adb: New test.
>         * gnat.dg/opt81_pkg.ads: New helper.
>
> --
> Eric Botcazou