Re: [PATCH 8/9] ipa-sra: Make scan_expr_access bail out on uninteresting expressions

[Martin Jambor <mjambor@suse.cz>]

Hi,

On Tue, Dec 13 2022, Richard Biener wrote:
> 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  <mjambor@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?
>

The patch was intended just as a simple optimization in order not to run
get_ref_base_and_extent on stuff where one can see from the top-most
tree they the result won't be interesting.  Indeed it looks like
get_ref_base_and_extent does not really need this when run on non-loads.

I'll think about the function a bit more but it seems like the patch
just is not really necessary.

Thanks,

Martin