diff mbox series

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

Message ID ri6wn6wh8bn.fsf@suse.cz
State New
Headers show
Series [1/9] ipa-cp: Write transformation summaries of all functions | expand

Commit Message

Martin Jambor Dec. 12, 2022, 4:53 p.m. UTC 
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(+)

Comments

Jan Hubicka Dec. 12, 2022, 9:56 p.m. UTC | #1 
> 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.

Honza
> +
>    base = get_ref_base_and_extent (expr, &poffset, &psize, &pmax_size, &reverse);
>  
>    if (TREE_CODE (base) == MEM_REF)
> -- 
> 2.38.1
>
Jan Hubicka Dec. 12, 2022, 9:58 p.m. UTC | #2 
> > 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.
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 Dec. 13, 2022, 8:40 a.m. UTC | #3 
> Am 12.12.2022 um 22:59 schrieb Jan Hubicka via Gcc-patches <gcc-patches@gcc.gnu.org>:
> 
> 
>> 
>>> 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.
> 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?

We can simplify the above to a single stripping of a handled component and considering another handled component as load (register ops are always single)

Richard 
> 
> 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 Dec. 13, 2022, 12:53 p.m. UTC | #4 
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?

> 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
> > > 
>
Martin Jambor Dec. 14, 2022, 1:20 p.m. UTC | #5 
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
diff mbox series

Patch

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;
+
   base = get_ref_base_and_extent (expr, &poffset, &psize, &pmax_size, &reverse);
 
   if (TREE_CODE (base) == MEM_REF)