On Fri, Nov 5, 2021 at 9:06 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 11/5/2021 9:09 AM, Aldy Hernandez wrote:
> > The main path discovery function was due for a cleanup.  First,
> > there's a nagging goto and second, my bitmap use was sloppy.  Hopefully
> > this makes the code easier for others to read.
> >
> > Regstrapped on x86-64 Linux.  I also made sure there were no difference
> > in the number of threads with this patch.
> >
> > No functional changes.
> >
> > OK?
> >
> > gcc/ChangeLog:
> >
> >       * tree-ssa-threadbackward.c (back_threader::find_paths_to_names):
> >       Remove gotos and other cleanups.
> > ---
> >   gcc/tree-ssa-threadbackward.c | 52 ++++++++++++++---------------------
> >   1 file changed, 20 insertions(+), 32 deletions(-)
> >
> > diff --git a/gcc/tree-ssa-threadbackward.c b/gcc/tree-ssa-threadbackward.c
> > index b7eaff94567..d6a5b0b8da2 100644
> > --- a/gcc/tree-ssa-threadbackward.c
> > +++ b/gcc/tree-ssa-threadbackward.c
> > @@ -402,26 +402,18 @@ back_threader::find_paths_to_names (basic_block bb, bitmap interesting)
> >
> >     m_path.safe_push (bb);
> >
> > +  // Try to resolve the path without looking back.
> >     if (m_path.length () > 1
> > -      && !m_profit.profitable_path_p (m_path, m_name, NULL))
> > +      && (!m_profit.profitable_path_p (m_path, m_name, NULL)
> > +       || maybe_register_path ()))
> >       {
> >         m_path.pop ();
> >         m_visited_bbs.remove (bb);
> >         return false;
> >       }
> >
> > -  // Try to resolve the path without looking back.
> > -  if (m_path.length () > 1 && maybe_register_path ())
> > -    {
> > -      m_path.pop ();
> > -      m_visited_bbs.remove (bb);
> > -      return true;
> > -    }
> Hmm, note the return values are different in these cases, which you've
> merged to always return false.    I was about to suggest you just kill
> the return value and the cleanups that would enable.  But I see your
> patch uses the return value where we didn't before.

Woah, good catch.

It looks like the return value is no longer needed, so it can indeed
be killed.  This was left over from when resolve_phi() didn't resolve
all incoming paths, but that's no longer the case.  Once we exit
find_path_to_names, all paths have been explored and there's nothing
to do.

OK pending tests?
Aldy

>
> So I think that raises the question, for the recursive calls which set
> "done", does the change in return value, particularly when
> maybe_register_path returns true impact anything?
>
> jeff
>