Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
On 15/07/2015 18:01, Martin Jambor wrote: > > So unless Martin objects consider the patch approved for trunk and for > > backporting after 5.2 is released and trunk shows no issues. > > > > Martin - can you take care of committing if you are fine with it? > > I have commitred the patch to trunk (and hopefully will not forget to > backport it once 5 branch reopens). I am testing the following > hopefully obvious followup, which I plan to commit tomorrow. It is > just mechanical renaming of the newly exported structures to give them > ipa prefix. Thanks Martin! Paolo
Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
Hi,
On Mon, Jul 13, 2015 at 03:49:05PM +0200, Richard Biener wrote:
> On Mon, Jul 13, 2015 at 3:46 PM, Paolo Bonzini wrote:
> >
> >
> > On 13/07/2015 15:45, Richard Biener wrote:
> >> It would be nice to have a patch that can be backported to the GCC 5 branch
> >> as well. We can improve this on trunk as followup,no?
> >
> > The patch I've already posted can be backported. O:-)
>
> So unless Martin objects consider the patch approved for trunk and for
> backporting
> after 5.2 is released and trunk shows no issues.
>
> Martin - can you take care of committing if you are fine with it?
>
I have commitred the patch to trunk (and hopefully will not forget to
backport it once 5 branch reopens). I am testing the following
hopefully obvious followup, which I plan to commit tomorrow. It is
just mechanical renaming of the newly exported structures to give them
ipa prefix.
Thanks,
Martin
2015-07-15 Martin Jambor
* ipa-prop.h (param_aa_status): Rename to ipa_param_aa_status. Adjust
all uses. Fix two typos in its general comment.
(func_body_info): Rename to ipa_func_body_info. Adjust all uses.
diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c
index 81a6860..b79ef14 100644
--- a/gcc/ipa-inline-analysis.c
+++ b/gcc/ipa-inline-analysis.c
@@ -1574,7 +1574,7 @@ unmodified_parm (gimple stmt, tree op)
loaded. */
static bool
-unmodified_parm_or_parm_agg_item (struct func_body_info *fbi,
+unmodified_parm_or_parm_agg_item (struct ipa_func_body_info *fbi,
gimple stmt, tree op, int *index_p,
struct agg_position_info *aggpos)
{
@@ -1745,7 +1745,7 @@ eliminated_by_inlining_prob (gimple stmt)
predicates to the CFG edges. */
static void
-set_cond_stmt_execution_predicate (struct func_body_info *fbi,
+set_cond_stmt_execution_predicate (struct ipa_func_body_info *fbi,
struct inline_summary *summary,
basic_block bb)
{
@@ -1827,7 +1827,7 @@ set_cond_stmt_execution_predicate (struct func_body_info
*fbi,
predicates to the CFG edges. */
static void
-set_switch_stmt_execution_predicate (struct func_body_info *fbi,
+set_switch_stmt_execution_predicate (struct ipa_func_body_info *fbi,
struct inline_summary *summary,
basic_block bb)
{
@@ -1888,7 +1888,7 @@ set_switch_stmt_execution_predicate (struct
func_body_info *fbi,
which it is executable. */
static void
-compute_bb_predicates (struct func_body_info *fbi,
+compute_bb_predicates (struct ipa_func_body_info *fbi,
struct cgraph_node *node,
struct inline_summary *summary)
{
@@ -2031,7 +2031,7 @@ will_be_nonconstant_expr_predicate (struct
ipa_node_params *info,
a compile time constant. */
static struct predicate
-will_be_nonconstant_predicate (struct func_body_info *fbi,
+will_be_nonconstant_predicate (struct ipa_func_body_info *fbi,
struct inline_summary *summary,
gimple stmt,
vec nonconstant_names)
@@ -2481,7 +2481,7 @@ estimate_function_body_sizes (struct cgraph_node *node,
bool early)
int freq;
struct inline_summary *info = inline_summaries->get (node);
struct predicate bb_predicate;
- struct func_body_info fbi;
+ struct ipa_func_body_info fbi;
vec nonconstant_names = vNULL;
int nblocks, n;
int *order;
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 615f749..2815ca6 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -481,7 +481,7 @@ ipa_set_ancestor_jf (struct ipa_jump_func *jfunc,
HOST_WIDE_INT offset,
of this function body. */
static struct ipa_bb_info *
-ipa_get_bb_info (struct func_body_info *fbi, basic_block bb)
+ipa_get_bb_info (struct ipa_func_body_info *fbi, basic_block bb)
{
gcc_checking_assert (fbi);
return &fbi->bb_infos[bb->index];
@@ -756,7 +756,7 @@ mark_modified (ao_ref *ao ATTRIBUTE_UNUSED, tree vdef
ATTRIBUTE_UNUSED,
should really just start giving up. */
static bool
-aa_overwalked (struct func_body_info *fbi)
+aa_overwalked (struct ipa_func_body_info *fbi)
{
gcc_checking_assert (fbi);
return fbi->aa_walked > (unsigned) PARAM_VALUE (PARAM_IPA_MAX_AA_STEPS);
@@ -765,8 +765,8 @@ aa_overwalked (struct func_body_info *fbi)
/* Find the nearest valid aa status for parameter specified by INDEX that
dominates BB. */
-static struct param_aa_status *
-find_dominating_aa_status (struct func_body_info *fbi, basic_block bb,
+static struct ipa_param_aa_status *
+find_dominating_aa_status (struct ipa_func_body_info *fbi, basic_block bb,
int index)
{
while (true)
@@ -785,21 +785,21 @@ find_dominating_aa_status (struct func_body_info *fbi,
basic_block bb,
structures and/or intialize the result with a dominating description as
necessary.
Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
Hi, On Mon, Jul 13, 2015 at 03:49:05PM +0200, Richard Biener wrote: > On Mon, Jul 13, 2015 at 3:46 PM, Paolo Bonzini wrote: > > > > > > On 13/07/2015 15:45, Richard Biener wrote: > >> It would be nice to have a patch that can be backported to the GCC 5 branch > >> as well. We can improve this on trunk as followup,no? > > > > The patch I've already posted can be backported. O:-) > > So unless Martin objects consider the patch approved for trunk and for > backporting > after 5.2 is released and trunk shows no issues. > > Martin - can you take care of committing if you are fine with it? I will. I will also do the renaming from func_body_info to ipa_func_body_info as a followup. Martin > > Thanks, > Richard. > > > Paolo
Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
On Mon, Jul 13, 2015 at 3:46 PM, Paolo Bonzini wrote: > > > On 13/07/2015 15:45, Richard Biener wrote: >> It would be nice to have a patch that can be backported to the GCC 5 branch >> as well. We can improve this on trunk as followup,no? > > The patch I've already posted can be backported. O:-) So unless Martin objects consider the patch approved for trunk and for backporting after 5.2 is released and trunk shows no issues. Martin - can you take care of committing if you are fine with it? Thanks, Richard. > Paolo
Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
On 13/07/2015 15:45, Richard Biener wrote: > It would be nice to have a patch that can be backported to the GCC 5 branch > as well. We can improve this on trunk as followup,no? The patch I've already posted can be backported. O:-) Paolo
Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
On Mon, Jul 13, 2015 at 3:30 PM, Martin Jambor wrote: > On Mon, Jul 13, 2015 at 02:47:23PM +0200, Paolo Bonzini wrote: >> >> >> On 13/07/2015 14:34, Martin Jambor wrote: >> > You might want to use Martin's shiny new >> > function_summary class in symbol-summary.c. That is a mechanism >> > specifically designed to append to a cgraph_node information specific >> > to an optimization pass (or two, as ipa-cp and ipa-inline already both >> > use a few of them). Unfortunately, the class is not very well >> > documented but you should be able to figure out how to use it from >> > other code using them. >> > >> > If you then always deallocate everything there at the end of >> > ipa-inline analysis, you'll get exactly the right life-time for the >> > data. >> >> Good. I might as well merge func_body_info and ipa_node_params then, so >> I already have ipa_node_params_sum. WDYT? > > Well, perhaps but I am not so sure. I have made a conscious decision > to make func_body_info a separate structure and not a part of > ipa_node_params exactly because of their different life-times. > > func_body_info is something only used during intra-procedural stage of > IPA analysis and should be thrown away as soon as it is over. > > ipa_node_params is a structure which contains results of that > analysis, which are streamed to disk during LTO and then read back for > the actual intEr-procedural propagation of information. Yes, it also > contains quite a few fields that are used only during the IPA stage > and so perhaps a few more bits used only during the intra-stage might > be OK too. But Honza recently told me the ipa-structures are > beginning to show in memory footprint of LTOing Firefox, so allocating > more unused memory for each and every function in Firefox and all its > clones is not really such a good idea. > > I also tend to think that coding the deallocation is going to be > easier for you if you just use another summary. For an > analysis-stage-only summary, you do not need to implement any of the > hooks (i.e. insert, remove, duplicate), for example. Those should > never happen during intraprocedural phase, or so I believe :-), so > just put gcc_unreachable into them and that should be it. > > I'm sorry for making this so complicated :-) It would be nice to have a patch that can be backported to the GCC 5 branch as well. We can improve this on trunk as followup,no? Richard. > Martin
Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
On Mon, Jul 13, 2015 at 02:47:23PM +0200, Paolo Bonzini wrote: > > > On 13/07/2015 14:34, Martin Jambor wrote: > > You might want to use Martin's shiny new > > function_summary class in symbol-summary.c. That is a mechanism > > specifically designed to append to a cgraph_node information specific > > to an optimization pass (or two, as ipa-cp and ipa-inline already both > > use a few of them). Unfortunately, the class is not very well > > documented but you should be able to figure out how to use it from > > other code using them. > > > > If you then always deallocate everything there at the end of > > ipa-inline analysis, you'll get exactly the right life-time for the > > data. > > Good. I might as well merge func_body_info and ipa_node_params then, so > I already have ipa_node_params_sum. WDYT? Well, perhaps but I am not so sure. I have made a conscious decision to make func_body_info a separate structure and not a part of ipa_node_params exactly because of their different life-times. func_body_info is something only used during intra-procedural stage of IPA analysis and should be thrown away as soon as it is over. ipa_node_params is a structure which contains results of that analysis, which are streamed to disk during LTO and then read back for the actual intEr-procedural propagation of information. Yes, it also contains quite a few fields that are used only during the IPA stage and so perhaps a few more bits used only during the intra-stage might be OK too. But Honza recently told me the ipa-structures are beginning to show in memory footprint of LTOing Firefox, so allocating more unused memory for each and every function in Firefox and all its clones is not really such a good idea. I also tend to think that coding the deallocation is going to be easier for you if you just use another summary. For an analysis-stage-only summary, you do not need to implement any of the hooks (i.e. insert, remove, duplicate), for example. Those should never happen during intraprocedural phase, or so I believe :-), so just put gcc_unreachable into them and that should be it. I'm sorry for making this so complicated :-) Martin
Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
On 13/07/2015 14:34, Martin Jambor wrote: > You might want to use Martin's shiny new > function_summary class in symbol-summary.c. That is a mechanism > specifically designed to append to a cgraph_node information specific > to an optimization pass (or two, as ipa-cp and ipa-inline already both > use a few of them). Unfortunately, the class is not very well > documented but you should be able to figure out how to use it from > other code using them. > > If you then always deallocate everything there at the end of > ipa-inline analysis, you'll get exactly the right life-time for the > data. Good. I might as well merge func_body_info and ipa_node_params then, so I already have ipa_node_params_sum. WDYT? Paolo
Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
Hi, On Mon, Jul 13, 2015 at 02:13:59PM +0200, Paolo Bonzini wrote: > > > On 13/07/2015 13:55, Martin Jambor wrote: > > I can't approve it, but FWIW, I'm generally fine with the patch. > > Although the original idea was to share one func_body_info in between > > ipa-cp and ipa-inline analyses, this is certainly better than what we > > have now and perhaps even good enough generally. > > Ah, so you'd add a pointer to cgraph_node? No, that does not seem right, given that this data is only needed during very limited time. You might want to use Martin's shiny new function_summary class in symbol-summary.c. That is a mechanism specifically designed to append to a cgraph_node information specific to an optimization pass (or two, as ipa-cp and ipa-inline already both use a few of them). Unfortunately, the class is not very well documented but you should be able to figure out how to use it from other code using them. If you then always deallocate everything there at the end of ipa-inline analysis, you'll get exactly the right life-time for the data. > I only have a question > then---does cgraph_node have a "destructor" where I can free the > func_body_info? Or is there no such thing? No, not really, but a summary would give you that. > > > The only semi-issue I have is the name of func_body_info. If it is > > going to be exposed in a header file, perhaps it should get an ipa_ > > prefix. > > That's a patch as large as this one. I can do the rename later, and > maybe have it preapproved to convince me to get the new public key in > place. :) Yeah, I think that is good enough (but remember I can't approve or preapprove anything). > > > I also think that its initialization should be put into a > > common function, but that is somethig I can do as a followup, if need > > be. > > Tried doing that now... it seems better to do it together with adding a > func_body_info* to cgraph_node*, so that the initialization is done > lazily in cgraph_node::get_func_body_info. > Well, see above, we're actually trying to pull information like this out of cgraph_node. Martin
Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
On 13/07/2015 13:55, Martin Jambor wrote: > I can't approve it, but FWIW, I'm generally fine with the patch. > Although the original idea was to share one func_body_info in between > ipa-cp and ipa-inline analyses, this is certainly better than what we > have now and perhaps even good enough generally. Ah, so you'd add a pointer to cgraph_node? I only have a question then---does cgraph_node have a "destructor" where I can free the func_body_info? Or is there no such thing? > The only semi-issue I have is the name of func_body_info. If it is > going to be exposed in a header file, perhaps it should get an ipa_ > prefix. That's a patch as large as this one. I can do the rename later, and maybe have it preapproved to convince me to get the new public key in place. :) > I also think that its initialization should be put into a > common function, but that is somethig I can do as a followup, if need > be. Tried doing that now... it seems better to do it together with adding a func_body_info* to cgraph_node*, so that the initialization is done lazily in cgraph_node::get_func_body_info. Paolo
Re: [PATCH] PR/66760, ipa-inline-analysis.c compile-time hog
Hi, On Sun, Jul 12, 2015 at 11:39:55PM +0200, Paolo Bonzini wrote: > From: bonz...@gnu.org > > In this PR, a lot of time is spent doing the same ipa_load_from_parm_agg > query over and over. Luckily a memoization scheme is already there, it's > just not used by ipa-inline-analysis.c. The patch moves the cache struct > (struct func_body_info) to ipa-prop.h and modify ipa-inline-analysis.c. > On some testcases from PR26854 the "alias stmt walking" timevar goes > off the profile while it used to be 30-70%. > > Bootstrapped (regtest in progress) on x86_64-pc-linux-gnu. > > Please commit the patch for me if approved, as I don't have anymore > the key I used to use for gcc.gnu.org. One of these days I'll send > my new SSH public key to the overseers. I can't approve it, but FWIW, I'm generally fine with the patch. Although the original idea was to share one func_body_info in between ipa-cp and ipa-inline analyses, this is certainly better than what we have now and perhaps even good enough generally. The only semi-issue I have is the name of func_body_info. If it is going to be exposed in a header file, perhaps it should get an ipa_ prefix. I also think that its initialization should be put into a common function, but that is somethig I can do as a followup, if need be. In any event, thanks for working on this, Martin
