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 ...
> -- >8 --
>
> While testing upcoming support for 64-bit location_t, I came across some
> test failures on sparc (32-bit) that trigger when location_t is changed to
> be 64-bit. The reason is that several call sites that make use of
> loop_version() for performing loop optimizations assume that a gphi*
> obtained prior to calling loop_version() will remain valid afterwards, but
> this is not the case for a PHI that needs to be resized. It doesn't happen
> usually, because PHI nodes usually have room for at least 4 arguments and
> this is usually more than are needed, but this is not guaranteed.
>
> Fix the affected callers by avoiding the assumption that a PHI node pointer
> remains valid. For most cases, this is done by remembering instead the
> gphi->result pointer, which contains a pointer back to the PHI node that is
> kept up to date when the PHI is moved to a new address.
>
> gcc/ChangeLog:
>
> * tree-parloops.cc (struct reduction_info): Store the result of the
> reduction PHI rather than the PHI itself.
> (reduction_info::reduc_phi): New member function.
> (reduction_hasher::equal): Adapt to the change in struct
> reduction_info.
> (reduction_phi): Likewise.
> (initialize_reductions): Likewise.
> (create_call_for_reduction_1): Likewise.
> (transform_to_exit_first_loop_alt): Likewise.
> (transform_to_exit_first_loop): Likewise.
> (build_new_reduction): Likewise.
> (set_reduc_phi_uids): Likewise.
> (try_create_reduction_list): Likewise.
> * tree-ssa-loop-split.cc (split_loop): Remember the PHI result
> variable so that the PHI can be found in case it is resized and move
> to a new address.
> * tree-vect-loop-manip.cc (vect_loop_versioning): After calling
> loop_version(), fix up stored PHI pointers in case they have
> changed.
> * tree-vectorizer.cc (vec_info::resync_stmt_addr): New function.
> * tree-vectorizer.h (vec_info::resync_stmt_addr): Declare.
> ---
> gcc/tree-parloops.cc | 40 +++++++++++++++++++++++--------------
> gcc/tree-ssa-loop-split.cc | 7 +++++++
> gcc/tree-vect-loop-manip.cc | 8 ++++++++
> gcc/tree-vectorizer.cc | 20 +++++++++++++++++++
> gcc/tree-vectorizer.h | 1 +
> 5 files changed, 61 insertions(+), 15 deletions(-)
>
> diff --git a/gcc/tree-parloops.cc b/gcc/tree-parloops.cc
> index 13d8e84bc8f..8427c287a6a 100644
> --- a/gcc/tree-parloops.cc
> +++ b/gcc/tree-parloops.cc
> @@ -895,7 +895,7 @@ parloops_force_simple_reduction (loop_vec_info loop_info,
> stmt_vec_info phi_info
> struct reduction_info
> {
> gimple *reduc_stmt; /* reduction statement. */
> - gimple *reduc_phi; /* The phi node defining the reduction. */
> + tree reduc_phi_name; /* The result of the phi node defining the
> reduction. */
> enum tree_code reduction_code;/* code for the reduction operation. */
> unsigned reduc_version; /* SSA_NAME_VERSION of original reduc_phi
> result. */
> @@ -910,6 +910,12 @@ struct reduction_info
> will be passed to the atomic operation.
> Represents
> the local result each thread computed for
> the reduction
> operation. */
> +
> + gphi *
> + reduc_phi () const
> + {
> + return as_a<gphi *> (SSA_NAME_DEF_STMT (reduc_phi_name));
> + }
> };
>
> /* Reduction info hashtable helpers. */
> @@ -925,7 +931,7 @@ struct reduction_hasher : free_ptr_hash <reduction_info>
> inline bool
> reduction_hasher::equal (const reduction_info *a, const reduction_info *b)
> {
> - return (a->reduc_phi == b->reduc_phi);
> + return (a->reduc_phi_name == b->reduc_phi_name);
> }
>
> inline hashval_t
> @@ -949,10 +955,10 @@ reduction_phi (reduction_info_table_type
> *reduction_list, gimple *phi)
> || gimple_uid (phi) == 0)
> return NULL;
>
> - tmpred.reduc_phi = phi;
> + tmpred.reduc_phi_name = gimple_phi_result (phi);
> tmpred.reduc_version = gimple_uid (phi);
> red = reduction_list->find (&tmpred);
> - gcc_assert (red == NULL || red->reduc_phi == phi);
> + gcc_assert (red == NULL || red->reduc_phi () == phi);
>
> return red;
> }
> @@ -1294,7 +1300,7 @@ initialize_reductions (reduction_info **slot, class
> loop *loop)
> from the preheader with the reduction initialization value. */
>
> /* Initialize the reduction. */
> - type = TREE_TYPE (PHI_RESULT (reduc->reduc_phi));
> + type = TREE_TYPE (reduc->reduc_phi_name);
> init = omp_reduction_init_op (gimple_location (reduc->reduc_stmt),
> reduc->reduction_code, type);
> reduc->init = init;
> @@ -1308,11 +1314,11 @@ initialize_reductions (reduction_info **slot, class
> loop *loop)
> computing is done. */
>
> e = loop_preheader_edge (loop);
> - arg = PHI_ARG_DEF_FROM_EDGE (reduc->reduc_phi, e);
> + const auto phi = reduc->reduc_phi ();
> + arg = PHI_ARG_DEF_FROM_EDGE (phi, e);
> /* Create new variable to hold the initial value. */
>
> - SET_USE (PHI_ARG_DEF_PTR_FROM_EDGE
> - (reduc->reduc_phi, loop_preheader_edge (loop)), init);
> + SET_USE (PHI_ARG_DEF_PTR_FROM_EDGE (phi, loop_preheader_edge (loop)),
> init);
> reduc->initial_value = arg;
> return 1;
> }
> @@ -1795,7 +1801,7 @@ create_call_for_reduction_1 (reduction_info **slot,
> struct clsn_data *clsn_data)
> {
> struct reduction_info *const reduc = *slot;
> gimple_stmt_iterator gsi;
> - tree type = TREE_TYPE (PHI_RESULT (reduc->reduc_phi));
> + tree type = TREE_TYPE (reduc->reduc_phi_name);
> tree load_struct;
> basic_block bb;
> basic_block new_bb;
> @@ -2424,8 +2430,9 @@ transform_to_exit_first_loop_alt (class loop *loop,
> if (red)
> {
> /* Register the new reduction phi. */
> - red->reduc_phi = nphi;
> - gimple_set_uid (red->reduc_phi, red->reduc_version);
> + red->reduc_phi_name = res_c;
> + gcc_checking_assert (red->reduc_phi () == nphi);
> + gimple_set_uid (nphi, red->reduc_version);
> }
> }
> gcc_assert (gsi_end_p (gsi) && !v->iterate (i, &vm));
> @@ -2651,6 +2658,8 @@ transform_to_exit_first_loop (class loop *loop,
> phi = gsi.phi ();
> res = PHI_RESULT (phi);
> t = copy_ssa_name (res, phi);
> + if (auto red = reduction_phi (reduction_list, phi))
> + red->reduc_phi_name = t;
> SET_PHI_RESULT (phi, t);
> nphi = create_phi_node (res, orig_header);
> add_phi_arg (nphi, t, hpred, UNKNOWN_LOCATION);
> @@ -3263,8 +3272,9 @@ build_new_reduction (reduction_info_table_type
> *reduction_list,
> new_reduction = XCNEW (struct reduction_info);
>
> new_reduction->reduc_stmt = reduc_stmt;
> - new_reduction->reduc_phi = phi;
> - new_reduction->reduc_version = SSA_NAME_VERSION (gimple_phi_result (phi));
> + const auto phi_name = gimple_phi_result (phi);
> + new_reduction->reduc_phi_name = phi_name;
> + new_reduction->reduc_version = SSA_NAME_VERSION (phi_name);
> new_reduction->reduction_code = reduction_code;
> slot = reduction_list->find_slot (new_reduction, INSERT);
> *slot = new_reduction;
> @@ -3276,7 +3286,7 @@ int
> set_reduc_phi_uids (reduction_info **slot, void *data ATTRIBUTE_UNUSED)
> {
> struct reduction_info *const red = *slot;
> - gimple_set_uid (red->reduc_phi, red->reduc_version);
> + gimple_set_uid (red->reduc_phi (), red->reduc_version);
> return 1;
> }
>
> @@ -3559,7 +3569,7 @@ try_create_reduction_list (loop_p loop,
> if (dump_file && (dump_flags & TDF_DETAILS))
> {
> fprintf (dump_file, "reduction phi is ");
> - print_gimple_stmt (dump_file, red->reduc_phi, 0);
> + print_gimple_stmt (dump_file, red->reduc_phi (), 0);
> fprintf (dump_file, "reduction stmt is ");
> print_gimple_stmt (dump_file, red->reduc_stmt, 0);
> }
> diff --git a/gcc/tree-ssa-loop-split.cc b/gcc/tree-ssa-loop-split.cc
> index 48d153d794a..a64066a9ae4 100644
> --- a/gcc/tree-ssa-loop-split.cc
> +++ b/gcc/tree-ssa-loop-split.cc
> @@ -622,6 +622,7 @@ split_loop (class loop *loop1)
> gphi *phi = find_or_create_guard_phi (loop1, guard_iv, &iv);
> if (!phi)
> continue;
> + const tree phi_result = gimple_phi_result (phi);
> gcond *guard_stmt = as_a<gcond *> (*gsi_last_bb (bbs[i]));
> tree guard_init = PHI_ARG_DEF_FROM_EDGE (phi,
> loop_preheader_edge (loop1));
> @@ -703,6 +704,12 @@ split_loop (class loop *loop1)
> profile_probability::very_likely (),
> true);
> gcc_assert (loop2);
> +
> + /* The phi address may have changed due to being resized in
> + loop_version (), so reobtain it. */
> + phi = as_a<gphi *> (SSA_NAME_DEF_STMT (phi_result));
> + gcc_checking_assert (gimple_bb (phi) == loop1->header);
> +
> /* Correct probability of edge cond_bb->preheader_of_loop2. */
> single_pred_edge
> (loop_preheader_edge (loop2)->src)->probability
> 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.
> +
> /* 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);
