| Message ID | d92f28d5-a7ef-34e5-044d-0ee04771a280@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | inline: improve internal function costs | expand |
On Thu, 1 Jun 2023, Andre Vieira (lists) wrote: > Hi, > > This is a follow-up of the internal function patch to add widening and > narrowing patterns. This patch improves the inliner cost estimation for > internal functions. I have no idea why calls are special in IPA analyze_function_body and so I cannot say whether treating all internal fn calls as non-calls is correct there. Honza? The tree-inline.cc change is OK though (you can push that separately). Thanks, Richard. > Bootstrapped and regression tested on aarch64-unknown-linux-gnu. > > gcc/ChangeLog: > > * ipa-fnsummary.cc (analyze_function_body): Correctly handle > non-zero costed internal functions. > * tree-inline.cc (estimate_num_insns): Improve costing for internal > functions. >
On 02/06/2023 10:13, Richard Biener wrote: > On Thu, 1 Jun 2023, Andre Vieira (lists) wrote: > >> Hi, >> >> This is a follow-up of the internal function patch to add widening and >> narrowing patterns. This patch improves the inliner cost estimation for >> internal functions. > > I have no idea why calls are special in IPA analyze_function_body > and so I cannot say whether treating all internal fn calls as > non-calls is correct there. Honza? > > The tree-inline.cc change is OK though (you can push that separately). I can't though, it ICEs on libgcc compilation (and other tests in testsuite). The estimate function is used by IPA to compute size and without the changes there it hits an assert because the estimate_num_insns no longer matches what ipa records in its size_time_table. I'll wait for Honza to comment. > > Thanks, > Richard. > >> Bootstrapped and regression tested on aarch64-unknown-linux-gnu. >> >> gcc/ChangeLog: >> >> * ipa-fnsummary.cc (analyze_function_body): Correctly handle >> non-zero costed internal functions. >> * tree-inline.cc (estimate_num_insns): Improve costing for internal >> functions. >> >
> On Thu, 1 Jun 2023, Andre Vieira (lists) wrote: > > > Hi, > > > > This is a follow-up of the internal function patch to add widening and > > narrowing patterns. This patch improves the inliner cost estimation for > > internal functions. > > I have no idea why calls are special in IPA analyze_function_body > and so I cannot say whether treating all internal fn calls as > non-calls is correct there. Honza? The reason is that normal statements are acconted as part of the function body, while calls have their costs attached to call edges (so it can be adjusted when call is inlined to otherwise optimized). However since internal functions have no cgraph edges, this looks like a bug that we do not test it. (the code was written before internal calls was introduced). I wonder if we don't want to have is_noninternal_gimple_call that could be used by IPA code to test whether cgraph edge should exist for the statement. > > The tree-inline.cc change is OK though (you can push that separately). The rest is OK too. Honza > > Thanks, > Richard. > > > Bootstrapped and regression tested on aarch64-unknown-linux-gnu. > > > > gcc/ChangeLog: > > > > * ipa-fnsummary.cc (analyze_function_body): Correctly handle > > non-zero costed internal functions. > > * tree-inline.cc (estimate_num_insns): Improve costing for internal > > functions. > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, > Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; > HRB 36809 (AG Nuernberg)
On 05/06/2023 04:04, Jan Hubicka wrote: >> On Thu, 1 Jun 2023, Andre Vieira (lists) wrote: >> >>> Hi, >>> >>> This is a follow-up of the internal function patch to add widening and >>> narrowing patterns. This patch improves the inliner cost estimation for >>> internal functions. >> >> I have no idea why calls are special in IPA analyze_function_body >> and so I cannot say whether treating all internal fn calls as >> non-calls is correct there. Honza? > > The reason is that normal statements are acconted as part of the > function body, while calls have their costs attached to call edges > (so it can be adjusted when call is inlined to otherwise optimized). > > However since internal functions have no cgraph edges, this looks like > a bug that we do not test it. (the code was written before internal > calls was introduced). > This sounds to me like you agree with my approach to treat internal calls different to regular calls. > I wonder if we don't want to have is_noninternal_gimple_call that could > be used by IPA code to test whether cgraph edge should exist for > the statement. I'm happy to add such a helper function @richi,rsandifo: you ok with that? >> >> The tree-inline.cc change is OK though (you can push that separately). > The rest is OK too. > Honza >> >> Thanks, >> Richard. >> >>> Bootstrapped and regression tested on aarch64-unknown-linux-gnu. >>> >>> gcc/ChangeLog: >>> >>> * ipa-fnsummary.cc (analyze_function_body): Correctly handle >>> non-zero costed internal functions. >>> * tree-inline.cc (estimate_num_insns): Improve costing for internal >>> functions. >>> >> >> -- >> Richard Biener <rguenther@suse.de> >> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, >> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; >> HRB 36809 (AG Nuernberg)
On Mon, 12 Jun 2023, Andre Vieira (lists) wrote: > > > On 05/06/2023 04:04, Jan Hubicka wrote: > >> On Thu, 1 Jun 2023, Andre Vieira (lists) wrote: > >> > >>> Hi, > >>> > >>> This is a follow-up of the internal function patch to add widening and > >>> narrowing patterns. This patch improves the inliner cost estimation for > >>> internal functions. > >> > >> I have no idea why calls are special in IPA analyze_function_body > >> and so I cannot say whether treating all internal fn calls as > >> non-calls is correct there. Honza? > > > > The reason is that normal statements are acconted as part of the > > function body, while calls have their costs attached to call edges > > (so it can be adjusted when call is inlined to otherwise optimized). > > > > However since internal functions have no cgraph edges, this looks like > > a bug that we do not test it. (the code was written before internal > > calls was introduced). > > > > This sounds to me like you agree with my approach to treat internal calls > different to regular calls. > > > I wonder if we don't want to have is_noninternal_gimple_call that could > > be used by IPA code to test whether cgraph edge should exist for > > the statement. > > I'm happy to add such a helper function @richi,rsandifo: you ok with that? It's a bit of an ugly name, if we want something that keys on calls that have an edge it should be obvious it does this. I wouldn't add is_noninternal_gimple_call. With LTO and libgcc and internal optab fns it's also less obvious in cases we want to have say .DIVMODDI3 (...) which in the end maps to a LTOed libcall from libgcc.a ... Richard.
diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc index b328bb8ce14b0725f6e5607da9d1e2f61e9baf62..449961fe44e4d86bf61e625dff0759d58e1e80ba 100644 --- a/gcc/ipa-fnsummary.cc +++ b/gcc/ipa-fnsummary.cc @@ -2862,16 +2862,19 @@ analyze_function_body (struct cgraph_node *node, bool early) to happen, but we cannot do that for call statements because edges are accounted specially. */ - if (*(is_gimple_call (stmt) ? &bb_predicate : &p) != false) + if (*(is_gimple_call (stmt) && !gimple_call_internal_p (stmt) + ? &bb_predicate : &p) != false) { time += final_time; size += this_size; } /* We account everything but the calls. Calls have their own - size/time info attached to cgraph edges. This is necessary - in order to make the cost disappear after inlining. */ - if (!is_gimple_call (stmt)) + size/time info attached to cgraph edges. This is necessary + in order to make the cost disappear after inlining. The only + exceptions are internal calls. */ + if (!is_gimple_call (stmt) + || gimple_call_internal_p (stmt)) { if (prob) { diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc index 99efddc36c8906a797583a569424336e961c35d1..bac84d277254703369c27993dcad048de8d4ff70 100644 --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -4427,7 +4427,48 @@ estimate_num_insns (gimple *stmt, eni_weights *weights) tree decl; if (gimple_call_internal_p (stmt)) - return 0; + { + switch (gimple_call_internal_fn (stmt)) + { + default: + return 1; + + case IFN_GOMP_TARGET_REV: + case IFN_GOMP_USE_SIMT: + case IFN_GOMP_SIMT_ENTER_ALLOC: + case IFN_GOMP_SIMT_EXIT: + case IFN_GOMP_SIMT_LANE: + case IFN_GOMP_SIMT_VF: + case IFN_GOMP_SIMT_LAST_LANE: + case IFN_GOMP_SIMT_ORDERED_PRED: + case IFN_GOMP_SIMT_VOTE_ANY: + case IFN_GOMP_SIMT_XCHG_BFLY: + case IFN_GOMP_SIMT_XCHG_IDX: + case IFN_GOMP_SIMD_LANE: + case IFN_GOMP_SIMD_VF: + case IFN_GOMP_SIMD_LAST_LANE: + case IFN_GOMP_SIMD_ORDERED_START: + case IFN_GOMP_SIMD_ORDERED_END: + case IFN_BUILTIN_EXPECT: + case IFN_ANNOTATE: + case IFN_NOP: + case IFN_UNIQUE: + case IFN_DEFERRED_INIT: + case IFN_ASSUME: + return 0; + + case IFN_UBSAN_NULL: + case IFN_UBSAN_BOUNDS: + case IFN_UBSAN_VPTR: + case IFN_UBSAN_CHECK_ADD: + case IFN_UBSAN_CHECK_SUB: + case IFN_UBSAN_CHECK_MUL: + case IFN_UBSAN_PTR: + case IFN_UBSAN_OBJECT_SIZE: + /* Estimating a compare and jump. */ + return 2; + } + } else if ((decl = gimple_call_fndecl (stmt)) && fndecl_built_in_p (decl)) {
Hi, This is a follow-up of the internal function patch to add widening and narrowing patterns. This patch improves the inliner cost estimation for internal functions. Bootstrapped and regression tested on aarch64-unknown-linux-gnu. gcc/ChangeLog: * ipa-fnsummary.cc (analyze_function_body): Correctly handle non-zero costed internal functions. * tree-inline.cc (estimate_num_insns): Improve costing for internal functions.