Re: [patch] Extend GIMPLE store merging to throwing stores

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Oct 1, 2019 at 1:05 PM Eric Botcazou <ebotcazou@adacore.com> wrote:
>
> [Thanks for the quick review and sorry for the longish delay]
>
> > +/* 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?
>
> Yes, I added it for C & C++, which both trivially fail the first test.  More
> generally, every additional processing is (directly or indirectly) guarded by
> the conjunction cfun->can_throw_non_call_exceptions && cfun->eh throughout.
>
> > +  /* 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?
>
> The latter.
>
> > Removing unreachable handlers is also to make things match better?
>
> Nope, only because calculate_dominance_info aborts otherwise below.
>
> > 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).
>
> This block of code is a manual, stripped down ehcleanup pass.
>
> > 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?
>
> The main point is the unsplitting though so this would trade an explicit call
> for a less implicit one.  But what I could do is to rename unsplit_all_eh into
> unsplit_all_eh_1 and hide the technicalities in a new unsplit_all_eh.

that works for me - the patch is OK with that change.

Thanks,
Richard.

> --
> Eric Botcazou