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);