On Thu, Apr 9, 2020 at 7:57 PM Martin Jambor <mjam...@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 <mjam...@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 <mjam...@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 <zsp...@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
>
