On Mon, 12 Dec 2022, Jan Hubicka wrote:
> > > Hi,
> > >
> > > I'm re-posting patches which I have posted at the end of stage 1 but
> > > which have not passed review yet.
> > >
> > > 8<--------------------------------------------------------------------
> > >
> > > I have noticed that scan_expr_access passes all the expressions it
> > > gets to get_ref_base_and_extent even when we are really only
> > > interested in memory accesses. So bail out when the expression is
> > > something clearly uninteresting.
> > >
> > > Bootstrapped and tested individually when I originally posted it and
> > > now bootstrapped and LTO-bootstrapped and tested as part of the whole
> > > series. OK for master?
> > >
> > >
> > > gcc/ChangeLog:
> > >
> > > 2021-12-14 Martin Jambor <mjam...@suse.cz>
> > >
> > > * ipa-sra.c (scan_expr_access): Bail out early if expr is something we
> > > clearly do not need to pass to get_ref_base_and_extent.
> > > ---
> > > gcc/ipa-sra.cc | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc
> > > index 93fceeafc73..3646d71468c 100644
> > > --- a/gcc/ipa-sra.cc
> > > +++ b/gcc/ipa-sra.cc
> > > @@ -1748,6 +1748,11 @@ scan_expr_access (tree expr, gimple *stmt,
> > > isra_scan_context ctx,
> > > || TREE_CODE (expr) == REALPART_EXPR)
> > > expr = TREE_OPERAND (expr, 0);
> > >
> > > + if (!handled_component_p (expr)
> > > + && !DECL_P (expr)
> > > + && TREE_CODE (expr) != MEM_REF)
> > > + return;
> > Is this needed because get_ref_base_and_extend crashes if given SSA_NAME
> > or something else or is it just optimization?
> > Perhaps Richi will know if there is better test for this.
Also the code preceeding the above
if (TREE_CODE (expr) == BIT_FIELD_REF
|| TREE_CODE (expr) == IMAGPART_EXPR
|| TREE_CODE (expr) == REALPART_EXPR)
expr = TREE_OPERAND (expr, 0);
but get_ref_base_and_extent shouldn't crash on anything here. The
question is what you want 'expr' to be? The comment of the function
says CTX specifies that, but doesn't constrain the CALL case (does
it have to be a memory argument)?
With allowing handled_component_p but above not handling
VIEW_CONVERT_EXPR you leave the possibility of VIEW_CONVERT_EXPR (d_1)
slipping through. Since the non-memory cases will have at most
one wrapping handled_component get_ref_base_and_extent should be
reasonably cheap, so maybe just cut off SSA_NAME, ADDR_EXPR and
CONSTANT_CLASS_P at the start of the function?
> Looking at:
>
> static inline bool
> gimple_assign_load_p (const gimple *gs)
> {
> tree rhs;
> if (!gimple_assign_single_p (gs))
> return false;
> rhs = gimple_assign_rhs1 (gs);
> if (TREE_CODE (rhs) == WITH_SIZE_EXPR)
> return true;
> rhs = get_base_address (rhs);
> return (DECL_P (rhs)
> || TREE_CODE (rhs) == MEM_REF || TREE_CODE (rhs) == TARGET_MEM_REF);
> }
>
> I wonder if we don't want to avoid get_base_address (which is loopy) and
> use same check and move it into a new predicate that is more convenient
> to use?
>
> Honza
> >
> > Honza
> > > +
> > > base = get_ref_base_and_extent (expr, &poffset, &psize, &pmax_size,
> > > &reverse);
> > >
> > > if (TREE_CODE (base) == MEM_REF)
> > > --
> > > 2.38.1
> > >
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
