On Thu, May 29, 2025 at 11:48 PM Andrew Pinski <pins...@gmail.com> wrote:
>
> On Tue, May 27, 2025 at 5:14 AM Richard Biener
> <richard.guent...@gmail.com> wrote:
> >
> > On Tue, May 27, 2025 at 5:02 AM Andrew Pinski <quic_apin...@quicinc.com>
> > wrote:
> > >
> > > This was noticed in the review of copy propagation for aggregates
> > > patch, instead of checking for a NULL or a non-ssa name of vuse,
> > > we should instead check if it the vuse is a default name and stop
> > > then.
> > >
> > > Bootstrapped and tested on x86_64-linux-gnu.
> > >
> > > gcc/ChangeLog:
> > >
> > > * tree-ssa-forwprop.cc (optimize_memcpy_to_memset): Change check
> > > from NULL/non-ssa name to default name.
> > >
> > > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> > > ---
> > > gcc/tree-ssa-forwprop.cc | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/gcc/tree-ssa-forwprop.cc b/gcc/tree-ssa-forwprop.cc
> > > index 4c048a9a298..e457a69ed48 100644
> > > --- a/gcc/tree-ssa-forwprop.cc
> > > +++ b/gcc/tree-ssa-forwprop.cc
> > > @@ -1226,7 +1226,8 @@ optimize_memcpy_to_memset (gimple_stmt_iterator
> > > *gsip, tree dest, tree src, tree
> > > gimple *defstmt;
> > > unsigned limit = param_sccvn_max_alias_queries_per_access;
> > > do {
> > > - if (vuse == NULL || TREE_CODE (vuse) != SSA_NAME)
> > > + /* If the vuse is the default definition, then there is no stores
> > > beforhand. */
> > > + if (SSA_NAME_IS_DEFAULT_DEF (vuse))
> >
> > Since forwprop does update_ssa in the end I was wondering whether any
> > bare non-SSA VUSE/VDEFs sneak in - for this the != SSA_NAME check
> > would be useful. On a GIMPLE stmt gimple_vuse () will return NULL
> > when it's not a load or store (or with a novops call), as you are using
> > gimple_store_p/gimple_assign_load_p there might be a disconnect
> > between those predicates and the presence of a vuse (I hope not, but ...)
> >
> > The patch looks OK to me, the comments above apply to the copy propagation
> > case.
>
> The copy prop case should be ok too since the vuse/vdef on the
> statement does not change when doing the prop; only the rhs of the
> statement. There is no inserting of a statement. This is unless we
> remove the statement and then unlink_stmt_vdef will prop the vuse into
> the vdef of the statement which we are removing.
>
> I did test the copy prop using just SSA_NAME_IS_DEFAULT_DEF and there
> were no regressions there either.
>
> When optimize_memcpy_to_memset was part of fold_stmt, a NULL vuse
> and/or a non-SSA vuse was common due to running before ssa. This was
> why there was a check for non-SSA.
> I am not sure why there was a check for NULLness was there when it was
> part of fold-all-builtins though.
>
> On a side note I think many passes have TODO_update_ssa on them when
> they already keep the ssa up to date now. I wonder if most of that
> dates from the days of VMUST_DEF/VMAY_DEF and multiple names on them
> rather than one virtual name.
Could be. TODO_update_ssa is cheap when nothing is to be done, but of
course it hides missed SSA updates. Getting rid of unnecessary ones would
be nice.
Richard.
>
> Thanks,
> Andrew
>
>
> >
> > Thanks,
> > Richard.
> >
> > > return false;
> > > defstmt = SSA_NAME_DEF_STMT (vuse);
> > > if (is_a <gphi*>(defstmt))
> > > --
> > > 2.43.0
> > >
