Re: [EXTERNAL] Re: [PATCH][PUSHED] Fix cfg maintenance after inlining in AutoFDO
On Wed, May 10, 2023 at 12:05 AM Eugene Rozenfeld wrote: > > >> free_dominance_info (CDI_DOMINATORS); @@ -1674,7 +1677,7 @@ > >> auto_profile (void) > >> pop_cfun (); > >>} > >> > >> - return TODO_rebuild_cgraph_edges; > >> + return 0; > > >This change isn't mentioned - was it accidential? > > No, it wasn't accidental. There is no reason to return > TODO_rebuild_cgraph_edges since we called cgraph_edge::rebuild_edges () after > each early_inline (). Ah, OK. Patch is still OK. Thanks, Richard. > Here is more context before that diff: > > todo |= early_inline (); > autofdo::afdo_annotate_cfg (promoted_stmts); > compute_function_frequency (); > > /* Local pure-const may imply need to fixup the cfg. */ > todo |= execute_fixup_cfg (); > if (todo & TODO_cleanup_cfg) > cleanup_tree_cfg (); > > free_dominance_info (CDI_DOMINATORS); > free_dominance_info (CDI_POST_DOMINATORS); > cgraph_edge::rebuild_edges (); > compute_fn_summary (cgraph_node::get (current_function_decl), true); > pop_cfun (); > } > > return 0; > > Thanks, > > Eugene > > -----Original Message----- > From: Richard Biener > Sent: Monday, May 8, 2023 11:40 PM > To: Eugene Rozenfeld > Cc: gcc-patches@gcc.gnu.org > Subject: [EXTERNAL] Re: [PATCH][PUSHED] Fix cfg maintenance after inlining in > AutoFDO > > On Tue, May 9, 2023 at 12:27 AM Eugene Rozenfeld via Gcc-patches > wrote: > > > > Todo from early_inliner needs to be propagated so that > > cleanup_tree_cfg () is called if necessary. > > > > This bug was causing an assert in get_loop_body during ipa-sra in > > autoprofiledbootstrap build since loops weren't fixed up and one of > > the loops had num_nodes set to 0. > > > > Tested on x86_64-pc-linux-gnu. > > > > gcc/ChangeLog: > > > > * auto-profile.cc (auto_profile): Check todo from early_inline > > to see if cleanup_tree_vfg needs to be called. > > _cfg > > > (early_inline): Return todo from early_inliner. > > --- > > gcc/auto-profile.cc | 21 - > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc index > > 360c42c4b89..e3af3555e75 100644 > > --- a/gcc/auto-profile.cc > > +++ b/gcc/auto-profile.cc > > @@ -1589,13 +1589,14 @@ afdo_annotate_cfg (const stmt_set > > _stmts) > > > > /* Wrapper function to invoke early inliner. */ > > > > -static void > > +static unsigned int > > early_inline () > > { > >compute_fn_summary (cgraph_node::get (current_function_decl), > > true); > > - unsigned todo = early_inliner (cfun); > > + unsigned int todo = early_inliner (cfun); > >if (todo & TODO_update_ssa_any) > > update_ssa (TODO_update_ssa); > > + return todo; > > } > > > > /* Use AutoFDO profile to annoate the control flow graph. > > @@ -1651,20 +1652,22 @@ auto_profile (void) > > function before annotation, so the profile inside bar@loc_foo2 > > will be useful. */ > > autofdo::stmt_set promoted_stmts; > > +unsigned int todo = 0; > > for (int i = 0; i < 10; i++) > >{ > > -if (!flag_value_profile_transformations > > -|| !autofdo::afdo_vpt_for_early_inline (_stmts)) > > - break; > > -early_inline (); > > + if (!flag_value_profile_transformations > > + || !autofdo::afdo_vpt_for_early_inline (_stmts)) > > + break; > > + todo |= early_inline (); > >} > > > > -early_inline (); > > +todo |= early_inline (); > > autofdo::afdo_annotate_cfg (promoted_stmts); > > compute_function_frequency (); > > > > /* Local pure-const may imply need to fixup the cfg. */ > > -if (execute_fixup_cfg () & TODO_cleanup_cfg) > > +todo |= execute_fixup_cfg (); > > +if (todo & TODO_cleanup_cfg) > >cleanup_tree_cfg (); > > > > free_dominance_info (CDI_DOMINATORS); @@ -1674,7 +1677,7 @@ > > auto_profile (void) > > pop_cfun (); > >} > > > > - return TODO_rebuild_cgraph_edges; > > + return 0; > > This change isn't mentioned - was it accidential? > > Otherwise looks OK. > > Thanks, > Richard. > > > } > > } /* namespace autofdo. */ > > > > -- > > 2.25.1 > >
RE: [EXTERNAL] Re: [PATCH][PUSHED] Fix cfg maintenance after inlining in AutoFDO
>> free_dominance_info (CDI_DOMINATORS); @@ -1674,7 +1677,7 @@ >> auto_profile (void) >> pop_cfun (); >>} >> >> - return TODO_rebuild_cgraph_edges; >> + return 0; >This change isn't mentioned - was it accidential? No, it wasn't accidental. There is no reason to return TODO_rebuild_cgraph_edges since we called cgraph_edge::rebuild_edges () after each early_inline (). Here is more context before that diff: todo |= early_inline (); autofdo::afdo_annotate_cfg (promoted_stmts); compute_function_frequency (); /* Local pure-const may imply need to fixup the cfg. */ todo |= execute_fixup_cfg (); if (todo & TODO_cleanup_cfg) cleanup_tree_cfg (); free_dominance_info (CDI_DOMINATORS); free_dominance_info (CDI_POST_DOMINATORS); cgraph_edge::rebuild_edges (); compute_fn_summary (cgraph_node::get (current_function_decl), true); pop_cfun (); } return 0; Thanks, Eugene -Original Message- From: Richard Biener Sent: Monday, May 8, 2023 11:40 PM To: Eugene Rozenfeld Cc: gcc-patches@gcc.gnu.org Subject: [EXTERNAL] Re: [PATCH][PUSHED] Fix cfg maintenance after inlining in AutoFDO On Tue, May 9, 2023 at 12:27 AM Eugene Rozenfeld via Gcc-patches wrote: > > Todo from early_inliner needs to be propagated so that > cleanup_tree_cfg () is called if necessary. > > This bug was causing an assert in get_loop_body during ipa-sra in > autoprofiledbootstrap build since loops weren't fixed up and one of > the loops had num_nodes set to 0. > > Tested on x86_64-pc-linux-gnu. > > gcc/ChangeLog: > > * auto-profile.cc (auto_profile): Check todo from early_inline > to see if cleanup_tree_vfg needs to be called. _cfg > (early_inline): Return todo from early_inliner. > --- > gcc/auto-profile.cc | 21 - > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc index > 360c42c4b89..e3af3555e75 100644 > --- a/gcc/auto-profile.cc > +++ b/gcc/auto-profile.cc > @@ -1589,13 +1589,14 @@ afdo_annotate_cfg (const stmt_set > _stmts) > > /* Wrapper function to invoke early inliner. */ > > -static void > +static unsigned int > early_inline () > { >compute_fn_summary (cgraph_node::get (current_function_decl), > true); > - unsigned todo = early_inliner (cfun); > + unsigned int todo = early_inliner (cfun); >if (todo & TODO_update_ssa_any) > update_ssa (TODO_update_ssa); > + return todo; > } > > /* Use AutoFDO profile to annoate the control flow graph. > @@ -1651,20 +1652,22 @@ auto_profile (void) > function before annotation, so the profile inside bar@loc_foo2 > will be useful. */ > autofdo::stmt_set promoted_stmts; > +unsigned int todo = 0; > for (int i = 0; i < 10; i++) >{ > -if (!flag_value_profile_transformations > -|| !autofdo::afdo_vpt_for_early_inline (_stmts)) > - break; > -early_inline (); > + if (!flag_value_profile_transformations > + || !autofdo::afdo_vpt_for_early_inline (_stmts)) > + break; > + todo |= early_inline (); >} > > -early_inline (); > +todo |= early_inline (); > autofdo::afdo_annotate_cfg (promoted_stmts); > compute_function_frequency (); > > /* Local pure-const may imply need to fixup the cfg. */ > -if (execute_fixup_cfg () & TODO_cleanup_cfg) > +todo |= execute_fixup_cfg (); > +if (todo & TODO_cleanup_cfg) >cleanup_tree_cfg (); > > free_dominance_info (CDI_DOMINATORS); @@ -1674,7 +1677,7 @@ > auto_profile (void) > pop_cfun (); >} > > - return TODO_rebuild_cgraph_edges; > + return 0; This change isn't mentioned - was it accidential? Otherwise looks OK. Thanks, Richard. > } > } /* namespace autofdo. */ > > -- > 2.25.1 >
Re: [PATCH][PUSHED] Fix cfg maintenance after inlining in AutoFDO
On Tue, May 9, 2023 at 12:27 AM Eugene Rozenfeld via Gcc-patches wrote: > > Todo from early_inliner needs to be propagated so that > cleanup_tree_cfg () is called if necessary. > > This bug was causing an assert in get_loop_body during > ipa-sra in autoprofiledbootstrap build since loops weren't > fixed up and one of the loops had num_nodes set to 0. > > Tested on x86_64-pc-linux-gnu. > > gcc/ChangeLog: > > * auto-profile.cc (auto_profile): Check todo from early_inline > to see if cleanup_tree_vfg needs to be called. _cfg > (early_inline): Return todo from early_inliner. > --- > gcc/auto-profile.cc | 21 - > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc > index 360c42c4b89..e3af3555e75 100644 > --- a/gcc/auto-profile.cc > +++ b/gcc/auto-profile.cc > @@ -1589,13 +1589,14 @@ afdo_annotate_cfg (const stmt_set _stmts) > > /* Wrapper function to invoke early inliner. */ > > -static void > +static unsigned int > early_inline () > { >compute_fn_summary (cgraph_node::get (current_function_decl), true); > - unsigned todo = early_inliner (cfun); > + unsigned int todo = early_inliner (cfun); >if (todo & TODO_update_ssa_any) > update_ssa (TODO_update_ssa); > + return todo; > } > > /* Use AutoFDO profile to annoate the control flow graph. > @@ -1651,20 +1652,22 @@ auto_profile (void) > function before annotation, so the profile inside bar@loc_foo2 > will be useful. */ > autofdo::stmt_set promoted_stmts; > +unsigned int todo = 0; > for (int i = 0; i < 10; i++) >{ > -if (!flag_value_profile_transformations > -|| !autofdo::afdo_vpt_for_early_inline (_stmts)) > - break; > -early_inline (); > + if (!flag_value_profile_transformations > + || !autofdo::afdo_vpt_for_early_inline (_stmts)) > + break; > + todo |= early_inline (); >} > > -early_inline (); > +todo |= early_inline (); > autofdo::afdo_annotate_cfg (promoted_stmts); > compute_function_frequency (); > > /* Local pure-const may imply need to fixup the cfg. */ > -if (execute_fixup_cfg () & TODO_cleanup_cfg) > +todo |= execute_fixup_cfg (); > +if (todo & TODO_cleanup_cfg) >cleanup_tree_cfg (); > > free_dominance_info (CDI_DOMINATORS); > @@ -1674,7 +1677,7 @@ auto_profile (void) > pop_cfun (); >} > > - return TODO_rebuild_cgraph_edges; > + return 0; This change isn't mentioned - was it accidential? Otherwise looks OK. Thanks, Richard. > } > } /* namespace autofdo. */ > > -- > 2.25.1 >
[PATCH][PUSHED] Fix cfg maintenance after inlining in AutoFDO
Todo from early_inliner needs to be propagated so that cleanup_tree_cfg () is called if necessary. This bug was causing an assert in get_loop_body during ipa-sra in autoprofiledbootstrap build since loops weren't fixed up and one of the loops had num_nodes set to 0. Tested on x86_64-pc-linux-gnu. gcc/ChangeLog: * auto-profile.cc (auto_profile): Check todo from early_inline to see if cleanup_tree_vfg needs to be called. (early_inline): Return todo from early_inliner. --- gcc/auto-profile.cc | 21 - 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc index 360c42c4b89..e3af3555e75 100644 --- a/gcc/auto-profile.cc +++ b/gcc/auto-profile.cc @@ -1589,13 +1589,14 @@ afdo_annotate_cfg (const stmt_set _stmts) /* Wrapper function to invoke early inliner. */ -static void +static unsigned int early_inline () { compute_fn_summary (cgraph_node::get (current_function_decl), true); - unsigned todo = early_inliner (cfun); + unsigned int todo = early_inliner (cfun); if (todo & TODO_update_ssa_any) update_ssa (TODO_update_ssa); + return todo; } /* Use AutoFDO profile to annoate the control flow graph. @@ -1651,20 +1652,22 @@ auto_profile (void) function before annotation, so the profile inside bar@loc_foo2 will be useful. */ autofdo::stmt_set promoted_stmts; +unsigned int todo = 0; for (int i = 0; i < 10; i++) { -if (!flag_value_profile_transformations -|| !autofdo::afdo_vpt_for_early_inline (_stmts)) - break; -early_inline (); + if (!flag_value_profile_transformations + || !autofdo::afdo_vpt_for_early_inline (_stmts)) + break; + todo |= early_inline (); } -early_inline (); +todo |= early_inline (); autofdo::afdo_annotate_cfg (promoted_stmts); compute_function_frequency (); /* Local pure-const may imply need to fixup the cfg. */ -if (execute_fixup_cfg () & TODO_cleanup_cfg) +todo |= execute_fixup_cfg (); +if (todo & TODO_cleanup_cfg) cleanup_tree_cfg (); free_dominance_info (CDI_DOMINATORS); @@ -1674,7 +1677,7 @@ auto_profile (void) pop_cfun (); } - return TODO_rebuild_cgraph_edges; + return 0; } } /* namespace autofdo. */ -- 2.25.1