Re: 0001-Don-t-skip-prologue-instructions-as-it-could-affect-.patch

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Jul 27, 2021 at 10:51 AM Bin.Cheng via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Mon, Jul 26, 2021 at 11:07 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
> >
> >
> >
> > On 7/25/2021 7:47 PM, Bin.Cheng wrote:
> > > On Sat, Jul 24, 2021 at 12:30 AM Jeff Law via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > >>
> > >>
> > >> On 7/14/2021 3:14 AM, bin.cheng via Gcc-patches wrote:
> > >>> Hi,
> > >>> I ran into a wrong code bug in code with deep template instantiation when working on sdx::simd.
> > >>> The root cause as described in commit summary is we skip prologue insns in init_alias_analysis.
> > >>> This simple patch fixes the issue, however, it's hard to reduce a case because of heavy use of
> > >>> templates.
> > >>> Bootstrap and test on x86_64, is it OK?
> > >> It's a clear correctness improvement, but what's unclear to me is why
> > >> we'd want to skip them in the epilogue either.
> > > I can only guess, there is nothing to initialize epilogue for because
> > > no code follows.
> > Yea, but couldn't the lack of analysis of the epilogue lead to a pass
> > mis-optimizing code within the epilogue itself?  It's not terribly
> > likely, but it just seems wrong to skip the epilogue like this.
> > Remember, the aliasing bits are just an analysis phase to find the
> > aliasing relationships that exist and we don't necessarily know how that
> > data is going to be used.  It may in fact be safe now, but may not be
> > safe in the future if someone added a late RTL pass that used the
> > aliasing info in a new way.
> >
> > The more I think about it, the more I think we should remove remove this
> > hunk of code completely.  There is some chance for fallout, but I think
> > it's unlikely.
> Hi Jeff,
> Thanks for the suggestion, here is the simple patch removing all of it.
> diff --git a/gcc/alias.c b/gcc/alias.c
> index 69e1eb89ac6..099acabca6b 100644
> --- a/gcc/alias.c
> +++ b/gcc/alias.c
> @@ -3406,14 +3406,6 @@ init_alias_analysis (void)
>    rpo = XNEWVEC (int, n_basic_blocks_for_fn (cfun));
>    rpo_cnt = pre_and_rev_post_order_compute (NULL, rpo, false);
>
> -  /* The prologue/epilogue insns are not threaded onto the
> -     insn chain until after reload has completed.  Thus,
> -     there is no sense wasting time checking if INSN is in
> -     the prologue/epilogue until after reload has completed.  */
> -  bool could_be_prologue_epilogue = ((targetm.have_prologue ()
> -                                     || targetm.have_epilogue ())
> -                                    && reload_completed);
> -
>    pass = 0;
>    do
>      {
> @@ -3459,10 +3451,6 @@ init_alias_analysis (void)
>                 {
>                   rtx note, set;
>
> -                 if (could_be_prologue_epilogue
> -                     && prologue_epilogue_contains (insn))
> -                   continue;
> -
>                   /* If this insn has a noalias note, process it,  Otherwise,
>                      scan for sets.  A simple set will have no side effects
>                      which could change the base value of any other
> register.  */
>
> No fallouts in bootstrap/test on x86_64.  Is it OK?

OK.

Thanks,
Richard.

> Thanks,
> bin
> >
> > Jeff
> >