On Sun, Jun 8, 2025 at 7:52 PM Andrew Pinski <quic_apin...@quicinc.com> wrote:
>
> While thinking about how to implement the rest of the copy prop and makes
> sure not
> to introduce some compile time problems, optimize_agr_copyprop should be
> changed
> into a forwproping rather than looking backwards.
Can you explain the issues when doing the backwards looking?
Note since this is a pure local "propagation" I still wonder whether
(now in the forward direction)
we want to use single_imm_use () instead of iterating over all uses of
the VDEF. Basically
we want to keep the optimization as "local" as possible.
I also wondered about
b = a;
c = b;
d = b;
we'll transform c = b into c = a and that's it.
In the end I still hope we can leverage more global analysis, like
that of SRA, to decide
whether propagation is profitable or not, thus whether we can elide a
temporary. In
that regard, shouldn't we avoid the propagation if 'DEST' has its
address taken given
we certainly cannot elide it?
Thanks,
Richard.
> Bootstrapped and tested on x86_64-linux-gnu.
>
> gcc/ChangeLog:
>
> * tree-ssa-forwprop.cc (optimize_agr_copyprop): Change into a
> forward looking (looking at vdef's uses) instead of a back
> looking (vuse's def).
>
> Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> ---
> gcc/tree-ssa-forwprop.cc | 121 +++++++++++++++++++--------------------
> 1 file changed, 60 insertions(+), 61 deletions(-)
>
> diff --git a/gcc/tree-ssa-forwprop.cc b/gcc/tree-ssa-forwprop.cc
> index 43b1c9d696f..2dc77ccba1d 100644
> --- a/gcc/tree-ssa-forwprop.cc
> +++ b/gcc/tree-ssa-forwprop.cc
> @@ -1344,12 +1344,12 @@ optimize_memcpy_to_memset (gimple_stmt_iterator
> *gsip, tree dest, tree src, tree
> return true;
> }
> /* Optimizes
> - a = c;
> - b = a;
> + DEST = SRC;
> + DEST2 = DEST; # DEST2 = SRC2;
> into
> - a = c;
> - b = c;
> - GSIP is the second statement and SRC is the common
> + DEST = SRC;
> + DEST2 = SRC;
> + GSIP is the first statement and SRC is the common
> between the statements.
> */
> static bool
> @@ -1365,65 +1365,64 @@ optimize_agr_copyprop (gimple_stmt_iterator *gsip)
> if (operand_equal_p (dest, src, 0))
> return false;
>
> - tree vuse = gimple_vuse (stmt);
> - /* If the vuse is the default definition, then there is no store
> beforehand. */
> - if (SSA_NAME_IS_DEFAULT_DEF (vuse))
> - return false;
> - gimple *defstmt = SSA_NAME_DEF_STMT (vuse);
> - if (!gimple_assign_load_p (defstmt)
> - || !gimple_store_p (defstmt))
> - return false;
> - if (gimple_has_volatile_ops (defstmt))
> - return false;
> -
> - tree dest2 = gimple_assign_lhs (defstmt);
> - tree src2 = gimple_assign_rhs1 (defstmt);
> -
> - /* If the original store is `src2 = src2;` skip over it. */
> - if (operand_equal_p (src2, dest2, 0))
> - return false;
> - if (!operand_equal_p (src, dest2, 0))
> - return false;
> -
> -
> - /* For 2 memory refences and using a temporary to do the copy,
> - don't remove the temporary as the 2 memory references might overlap.
> - Note t does not need to be decl as it could be field.
> - See PR 22237 for full details.
> - E.g.
> - t = *a;
> - *b = t;
> - Cannot be convert into
> - t = *a;
> - *b = *a;
> - Though the following is allowed to be done:
> - t = *a;
> - *a = t;
> - And convert it into:
> - t = *a;
> - *a = *a;
> - */
> - if (!operand_equal_p (src2, dest, 0)
> - && !DECL_P (dest) && !DECL_P (src2))
> - return false;
> -
> - if (dump_file && (dump_flags & TDF_DETAILS))
> + tree vdef = gimple_vdef (stmt);
> + imm_use_iterator iter;
> + gimple *use_stmt;
> + bool changed = false;
> + FOR_EACH_IMM_USE_STMT (use_stmt, iter, vdef)
> {
> - fprintf (dump_file, "Simplified\n ");
> - print_gimple_stmt (dump_file, stmt, 0, dump_flags);
> - fprintf (dump_file, "after previous\n ");
> - print_gimple_stmt (dump_file, defstmt, 0, dump_flags);
> - }
> - gimple_assign_set_rhs_from_tree (gsip, unshare_expr (src2));
> - update_stmt (stmt);
> + if (!gimple_assign_load_p (use_stmt)
> + || !gimple_store_p (use_stmt))
> + continue;
> + if (gimple_has_volatile_ops (use_stmt))
> + continue;
> + tree dest2 = gimple_assign_lhs (use_stmt);
> + tree src2 = gimple_assign_rhs1 (use_stmt);
> + /* If the new store is `src2 = src2;` skip over it. */
> + if (operand_equal_p (src2, dest2, 0))
> + continue;
> + if (!operand_equal_p (dest, src2, 0))
> + continue;
> + /* For 2 memory refences and using a temporary to do the copy,
> + don't remove the temporary as the 2 memory references might overlap.
> + Note t does not need to be decl as it could be field.
> + See PR 22237 for full details.
> + E.g.
> + t = *a; #DEST = SRC;
> + *b = t; #DEST2 = SRC2;
> + Cannot be convert into
> + t = *a;
> + *b = *a;
> + Though the following is allowed to be done:
> + t = *a;
> + *a = t;
> + And convert it into:
> + t = *a;
> + *a = *a;
> + */
> + if (!operand_equal_p (dest2, src, 0)
> + && !DECL_P (dest2) && !DECL_P (src))
> + continue;
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> + fprintf (dump_file, "Simplified\n ");
> + print_gimple_stmt (dump_file, use_stmt, 0, dump_flags);
> + fprintf (dump_file, "after previous\n ");
> + print_gimple_stmt (dump_file, stmt, 0, dump_flags);
> + }
> + gimple_stmt_iterator gsi = gsi_for_stmt (use_stmt);
> + gimple_assign_set_rhs_from_tree (&gsi, unshare_expr (src));
> + update_stmt (use_stmt);
>
> - if (dump_file && (dump_flags & TDF_DETAILS))
> - {
> - fprintf (dump_file, "into\n ");
> - print_gimple_stmt (dump_file, stmt, 0, dump_flags);
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> + fprintf (dump_file, "into\n ");
> + print_gimple_stmt (dump_file, use_stmt, 0, dump_flags);
> + }
> + statistics_counter_event (cfun, "copy prop for aggregate", 1);
> + changed = true;
> }
> - statistics_counter_event (cfun, "copy prop for aggregate", 1);
> - return true;
> + return changed;
> }
>
> /* *GSI_P is a GIMPLE_CALL to a builtin function.
> --
> 2.43.0
>
