| Message ID | ri6pncgzgmm.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | ipa-sra: Fix treatment of internal functions (PR 94434) | expand |
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 >
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);