On Tue, Dec 3, 2024 at 2:42 PM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Tue, Dec 3, 2024 at 2:07 PM Lewis Hyatt <lhy...@gmail.com> wrote:
> >
> > On Tue, Dec 03, 2024 at 01:28:28PM +0100, Richard Biener wrote:
> > > On Mon, Dec 2, 2024 at 1:55 AM Lewis Hyatt <lhy...@gmail.com> wrote:
> > > >
> > > > This patch is my new way to handle what was previously done in v2 patch
> > > > 04/14, discussed at:
> > > > https://gcc.gnu.org/pipermail/gcc-patches/2024-November/669527.html
> > > >
> > > > The only places I have found running into trouble with reallocated PHI 
> > > > nodes
> > > > (at least, the only places revealed by changing the size of location_t) 
> > > > are
> > > > those which remember a gphi object before a call to loop_version() and 
> > > > then
> > > > try to use it after. I fixed three affected call sites in this new 
> > > > patch.
> > >
> > > I guess it might be nice to avoid PHI node re-allocation by 
> > > loop_version(),
> > > I remember fixing up other CFG manipulations to avoid the need for
> > > intermittent more PHI arguments.  Anway, the patch is OK besides one
> > > minor adjustment, see below ...
> > >
> >
> > Thanks again for looking at all the patches. Comment below...
> >
> > ...
> >
> > > > diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc
> > > > index 54b29540eb5..4f3abb94e6a 100644
> > > > --- a/gcc/tree-vect-loop-manip.cc
> > > > +++ b/gcc/tree-vect-loop-manip.cc
> > > > @@ -4190,6 +4190,14 @@ vect_loop_versioning (loop_vec_info loop_vinfo,
> > > >                             prob * prob2, (prob * prob2).invert (),
> > > >                             prob * prob2, (prob * prob2).invert (),
> > > >                             true);
> > > > +
> > > > +      /* If the PHI nodes in the loop header were reallocated, we need 
> > > > to fix up
> > > > +        our internally stashed copies of those.  */
> > > > +      if (loop_to_version == loop)
> > > > +       for (auto gsi = gsi_start_phis (loop->header);
> > > > +            !gsi_end_p (gsi); gsi_next (&gsi))
> > > > +         loop_vinfo->resync_stmt_addr (gsi.phi ());
> > >
> > > Instead of a new resync_stmt_addr method can you simply do
> > >
> > >             if (stmt_vec_info s = loop_vinfo->lookup_stmt (gsi.phi ())
> > >               STMT_VINFO_STMT (s) = gsi.phi ();
> > >
> > > please?
> > >
> > > OK with that change.
> > >
> > > Thanks,
> > > Richard.
> >
> > The reason I added the new function is that lookup_stmt() does:
> >
> >       if (res && res->stmt == stmt)
> >         return res;
> >
> > so it returns NULL if the stored stmt does not match the one that was looked
> > up.
>
> Doh, I wonder what we added that for!
>
> > I could add a "bool relaxed = false" argument to lookup_stmt() instead,
> > or could add an alternate function lookup_stmt_relaxed() if you prefer? I
> > guess the second option would be pretty similar to what I did with
> > resync_stmt_addr(), just it would not actually make the change within the
> > function itself.
>
> No, in this case your patch is OK with an added comment (in your new function)
> why you can't use lookup_stmt.

Btw, I tried to look into why loop_version would ever need more PHI capacity
temporarily - so I did the following but it never triggers during bootstrap or
regtest.  That said - do you have a testcase that exercises this path?

diff --git a/gcc/tree-vectorizer.cc b/gcc/tree-vectorizer.cc
index 92ecd881508..bc3c6254fdf 100644
--- a/gcc/tree-vectorizer.cc
+++ b/gcc/tree-vectorizer.cc
@@ -556,6 +556,7 @@ vec_info::resync_stmt_addr (gimple *stmt)
       stmt_vec_info res = stmt_vec_infos[uid - 1];
       if (res && res->stmt)
        {
+         gcc_assert (res->stmt == stmt);
          res->stmt = stmt;
          return res;
        }


> Richard.
>
> >
> > >
> > > > +
> > > >        /* We will later insert second conditional so overall outcome of
> > > >          both is prob * prob2.  */
> > > >        edge true_e, false_e;
> > > > diff --git a/gcc/tree-vectorizer.cc b/gcc/tree-vectorizer.cc
> > > > index 9a068a40864..c8bb0fe14fc 100644
> > > > --- a/gcc/tree-vectorizer.cc
> > > > +++ b/gcc/tree-vectorizer.cc
> > > > @@ -541,6 +541,26 @@ vec_info::add_pattern_stmt (gimple *stmt, 
> > > > stmt_vec_info stmt_info)
> > > >    return res;
> > > >  }
> > > >
> > > > +/* If STMT was previously associated with a stmt_vec_info and STMT now 
> > > > resides
> > > > +   at a different address than before (e.g., because STMT is a phi 
> > > > node that has
> > > > +   been resized), update the expected address to match the new one.  */
> > > > +
> > > > +stmt_vec_info
> > > > +vec_info::resync_stmt_addr (gimple *stmt)
> > > > +{
> > > > +  unsigned int uid = gimple_uid (stmt);
> > > > +  if (uid > 0 && uid - 1 < stmt_vec_infos.length ())
> > > > +    {
> > > > +      stmt_vec_info res = stmt_vec_infos[uid - 1];
> > > > +      if (res && res->stmt)
> > > > +       {
> > > > +         res->stmt = stmt;
> > > > +         return res;
> > > > +       }
> > > > +    }
> > > > +  return nullptr;
> > > > +}
> > > > +
> > > >  /* If STMT has an associated stmt_vec_info, return that vec_info, 
> > > > otherwise
> > > >     return null.  It is safe to call this function on any statement, 
> > > > even if
> > > >     it might not be part of the vectorizable region.  */
> > > > diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
> > > > index 7f69a3f57b4..749a9299d35 100644
> > > > --- a/gcc/tree-vectorizer.h
> > > > +++ b/gcc/tree-vectorizer.h
> > > > @@ -500,6 +500,7 @@ public:
> > > >
> > > >    stmt_vec_info add_stmt (gimple *);
> > > >    stmt_vec_info add_pattern_stmt (gimple *, stmt_vec_info);
> > > > +  stmt_vec_info resync_stmt_addr (gimple *);
> > > >    stmt_vec_info lookup_stmt (gimple *);
> > > >    stmt_vec_info lookup_def (tree);
> > > >    stmt_vec_info lookup_single_use (tree);

Reply via email to