ipa-sra: Fix treatment of internal functions (PR 94434)

Message ID ri6pncgzgmm.fsf@suse.cz
State New
Headers show
Series
  • ipa-sra: Fix treatment of internal functions (PR 94434)
Related show

Commit Message

Martin Jambor April 9, 2020, 5:57 p.m.
Hi,

On Tue, Apr 07 2020, bule wrote:
> Hi, 

>

> The patch is tested and works fine. It is more appropriate to handle

> the context by considering it as a section of assemble code.

>

> A minor question is that I think svst functions are for store

>operations. Why pass ISRA_CTX_LOAD to scan_expr_access rather than

>ISRA_CTX_STORE?

>


That's a good point, I did not immediately realize that these were
writes.  IPA-SRA really distinguishes between reads and writes only when
it comes to parameters passed by reference but I agree that we should
not lie to the bit of the compiler (if we can avoid it :-).

Therefore I'd like to fix this issue with the patch below.  I'd
encourage anyone with experience with adding SVE testcases to add one
specifically for this.

Thanks,

Martin


ipa-sra: Fix treatment of internal functions (PR 94434)

IPA-SRA can segfault when processing a coll to an internal function
because such calls do not have corresponding call graphs and should be
treated like memory accesses anyway, which what the patch below does.

The patch makes an attempt to differentiate between reads and writes,
although for things passed by value it does not make any difference.  It
treats all arguments of functions denoted with internal_store_fn_p as
written to, even though in practice only some are, but for IPA-SRA
purposes, actions needed to be taken when processing a read are also
always performed when analyzing a write, so the code is just slightly
pessimistic.  But not as pessimistic as bailing out on any internal call
immediately.

I have LTO bootstrapped and tested the patch on x86_64-linux and
normally bootstrapped and tested it on aarch64-linux, although one
which is not SVE capable.  I would appreciate testing on such machine
too - as well as a small testcase that would follow all relevant
conventions in gcc.target/aarch64/sve.

OK for trunk?


2020-04-09  Martin Jambor  <mjambor@suse.cz>

	PR ipa/94434
	* ipa-sra.c: Include internal-fn.h.
	(enum isra_scan_context): Update comment.
	(scan_function): Treat calls to internal_functions like loads or stores.
---
 gcc/ChangeLog |  7 +++++++
 gcc/ipa-sra.c | 26 ++++++++++++++++++++------
 2 files changed, 27 insertions(+), 6 deletions(-)

-- 
2.26.0

Comments

Xi Ruoyao via Gcc-patches April 14, 2020, 8:31 a.m. | #1
On Thu, Apr 9, 2020 at 7:57 PM Martin Jambor <mjambor@suse.cz> wrote:
>

> Hi,

>

> On Tue, Apr 07 2020, bule wrote:

> > Hi,

> >

> > The patch is tested and works fine. It is more appropriate to handle

> > the context by considering it as a section of assemble code.

> >

> > A minor question is that I think svst functions are for store

> >operations. Why pass ISRA_CTX_LOAD to scan_expr_access rather than

> >ISRA_CTX_STORE?

> >

>

> That's a good point, I did not immediately realize that these were

> writes.  IPA-SRA really distinguishes between reads and writes only when

> it comes to parameters passed by reference but I agree that we should

> not lie to the bit of the compiler (if we can avoid it :-).

>

> Therefore I'd like to fix this issue with the patch below.  I'd

> encourage anyone with experience with adding SVE testcases to add one

> specifically for this.


OK.

Richard.

> Thanks,

>

> Martin

>

>

> ipa-sra: Fix treatment of internal functions (PR 94434)

>

> IPA-SRA can segfault when processing a coll to an internal function

> because such calls do not have corresponding call graphs and should be

> treated like memory accesses anyway, which what the patch below does.

>

> The patch makes an attempt to differentiate between reads and writes,

> although for things passed by value it does not make any difference.  It

> treats all arguments of functions denoted with internal_store_fn_p as

> written to, even though in practice only some are, but for IPA-SRA

> purposes, actions needed to be taken when processing a read are also

> always performed when analyzing a write, so the code is just slightly

> pessimistic.  But not as pessimistic as bailing out on any internal call

> immediately.

>

> I have LTO bootstrapped and tested the patch on x86_64-linux and

> normally bootstrapped and tested it on aarch64-linux, although one

> which is not SVE capable.  I would appreciate testing on such machine

> too - as well as a small testcase that would follow all relevant

> conventions in gcc.target/aarch64/sve.

>

> OK for trunk?

>

>

> 2020-04-09  Martin Jambor  <mjambor@suse.cz>

>

>         PR ipa/94434

>         * ipa-sra.c: Include internal-fn.h.

>         (enum isra_scan_context): Update comment.

>         (scan_function): Treat calls to internal_functions like loads or stores.

> ---

>  gcc/ChangeLog |  7 +++++++

>  gcc/ipa-sra.c | 26 ++++++++++++++++++++------

>  2 files changed, 27 insertions(+), 6 deletions(-)

>

> diff --git a/gcc/ChangeLog b/gcc/ChangeLog

> index e10fb251c16..a838634b707 100644

> --- a/gcc/ChangeLog

> +++ b/gcc/ChangeLog

> @@ -1,3 +1,10 @@

> +2020-04-09  Martin Jambor  <mjambor@suse.cz>

> +

> +       PR ipa/94434

> +       * ipa-sra.c: Include internal-fn.h.

> +       (enum isra_scan_context): Update comment.

> +       (scan_function): Treat calls to internal_functions like loads or stores.

> +

>  2020-04-05 Zachary Spytz  <zspytz@gmail.com>

>

>         * extend.texi: Add free to list of ISO C90 functions that

> diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c

> index f0ebaec708d..7c922e40a4e 100644

> --- a/gcc/ipa-sra.c

> +++ b/gcc/ipa-sra.c

> @@ -83,7 +83,7 @@ along with GCC; see the file COPYING3.  If not see

>  #include "builtins.h"

>  #include "cfganal.h"

>  #include "tree-streamer.h"

> -

> +#include "internal-fn.h"

>

>  /* Bits used to track size of an aggregate in bytes interprocedurally.  */

>  #define ISRA_ARG_SIZE_LIMIT_BITS 16

> @@ -1281,7 +1281,9 @@ allocate_access (gensum_param_desc *desc,

>  }

>

>  /* In what context scan_expr_access has been called, whether it deals with a

> -   load, a function argument, or a store.  */

> +   load, a function argument, or a store.  Please note that in rare

> +   circumstances when it is not clear if the access is a load or store,

> +   ISRA_CTX_STORE is used too.  */

>

>  enum isra_scan_context {ISRA_CTX_LOAD, ISRA_CTX_ARG, ISRA_CTX_STORE};

>

> @@ -1870,15 +1872,27 @@ scan_function (cgraph_node *node, struct function *fun)

>             case GIMPLE_CALL:

>               {

>                 unsigned argument_count = gimple_call_num_args (stmt);

> -               scan_call_info call_info;

> -               call_info.cs = node->get_edge (stmt);

> -               call_info.argument_count = argument_count;

> +               isra_scan_context ctx = ISRA_CTX_ARG;

> +               scan_call_info call_info, *call_info_p = &call_info;

> +               if (gimple_call_internal_p (stmt))

> +                 {

> +                   call_info_p = NULL;

> +                   ctx = ISRA_CTX_LOAD;

> +                   internal_fn ifn = gimple_call_internal_fn (stmt);

> +                   if (internal_store_fn_p (ifn))

> +                     ctx = ISRA_CTX_STORE;

> +                 }

> +               else

> +                 {

> +                   call_info.cs = node->get_edge (stmt);

> +                   call_info.argument_count = argument_count;

> +                 }

>

>                 for (unsigned i = 0; i < argument_count; i++)

>                   {

>                     call_info.arg_idx = i;

>                     scan_expr_access (gimple_call_arg (stmt, i), stmt,

> -                                     ISRA_CTX_ARG, bb, &call_info);

> +                                     ctx, bb, call_info_p);

>                   }

>

>                 tree lhs = gimple_call_lhs (stmt);

> --

> 2.26.0

>

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index e10fb251c16..a838634b707 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@ 
+2020-04-09  Martin Jambor  <mjambor@suse.cz>
+
+	PR ipa/94434
+	* ipa-sra.c: Include internal-fn.h.
+	(enum isra_scan_context): Update comment.
+	(scan_function): Treat calls to internal_functions like loads or stores.
+
 2020-04-05 Zachary Spytz  <zspytz@gmail.com>
 
 	* extend.texi: Add free to list of ISO C90 functions that
diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c
index f0ebaec708d..7c922e40a4e 100644
--- a/gcc/ipa-sra.c
+++ b/gcc/ipa-sra.c
@@ -83,7 +83,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "builtins.h"
 #include "cfganal.h"
 #include "tree-streamer.h"
-
+#include "internal-fn.h"
 
 /* Bits used to track size of an aggregate in bytes interprocedurally.  */
 #define ISRA_ARG_SIZE_LIMIT_BITS 16
@@ -1281,7 +1281,9 @@  allocate_access (gensum_param_desc *desc,
 }
 
 /* In what context scan_expr_access has been called, whether it deals with a
-   load, a function argument, or a store.  */
+   load, a function argument, or a store.  Please note that in rare
+   circumstances when it is not clear if the access is a load or store,
+   ISRA_CTX_STORE is used too.  */
 
 enum isra_scan_context {ISRA_CTX_LOAD, ISRA_CTX_ARG, ISRA_CTX_STORE};
 
@@ -1870,15 +1872,27 @@  scan_function (cgraph_node *node, struct function *fun)
 	    case GIMPLE_CALL:
 	      {
 		unsigned argument_count = gimple_call_num_args (stmt);
-		scan_call_info call_info;
-		call_info.cs = node->get_edge (stmt);
-		call_info.argument_count = argument_count;
+		isra_scan_context ctx = ISRA_CTX_ARG;
+		scan_call_info call_info, *call_info_p = &call_info;
+		if (gimple_call_internal_p (stmt))
+		  {
+		    call_info_p = NULL;
+		    ctx = ISRA_CTX_LOAD;
+		    internal_fn ifn = gimple_call_internal_fn (stmt);
+		    if (internal_store_fn_p (ifn))
+		      ctx = ISRA_CTX_STORE;
+		  }
+		else
+		  {
+		    call_info.cs = node->get_edge (stmt);
+		    call_info.argument_count = argument_count;
+		  }
 
 		for (unsigned i = 0; i < argument_count; i++)
 		  {
 		    call_info.arg_idx = i;
 		    scan_expr_access (gimple_call_arg (stmt, i), stmt,
-				      ISRA_CTX_ARG, bb, &call_info);
+				      ctx, bb, call_info_p);
 		  }
 
 		tree lhs = gimple_call_lhs (stmt);