Message ID | DS7PR21MB34793E5E1EA2C050CB45B2A691719@DS7PR21MB3479.namprd21.prod.outlook.com |
---|---|
State | New |
Headers | show |
Series | [PUSHED] Fix cfg maintenance after inlining in AutoFDO | expand |
On Tue, May 9, 2023 at 12:27 AM Eugene Rozenfeld via Gcc-patches <gcc-patches@gcc.gnu.org> 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 &promoted_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 (&promoted_stmts)) > - break; > - early_inline (); > + if (!flag_value_profile_transformations > + || !autofdo::afdo_vpt_for_early_inline (&promoted_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 >
>> 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 <richard.guenther@gmail.com> Sent: Monday, May 8, 2023 11:40 PM To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com> 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 <gcc-patches@gcc.gnu.org> 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 > &promoted_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 (&promoted_stmts)) > - break; > - early_inline (); > + if (!flag_value_profile_transformations > + || !autofdo::afdo_vpt_for_early_inline (&promoted_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 >
On Wed, May 10, 2023 at 12:05 AM Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com> 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 <richard.guenther@gmail.com> > Sent: Monday, May 8, 2023 11:40 PM > To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com> > 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 <gcc-patches@gcc.gnu.org> 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 > > &promoted_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 (&promoted_stmts)) > > - break; > > - early_inline (); > > + if (!flag_value_profile_transformations > > + || !autofdo::afdo_vpt_for_early_inline (&promoted_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 > >
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 &promoted_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 (&promoted_stmts)) - break; - early_inline (); + if (!flag_value_profile_transformations + || !autofdo::afdo_vpt_for_early_inline (&promoted_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. */