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 <jeffreya...@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 > >