| Message ID | 20200925220424.GA57232@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Fix handling of gimple_clobber in ipa_modref | expand |
On September 26, 2020 12:04:24 AM GMT+02:00, Jan Hubicka <hubicka@ucw.cz> wrote: >Hi, >while adding check for gimple_clobber I reversed the return value >so instead of ignoring the statement ipa-modref gives up. Fixed thus. >This explains the drop between originally reported disambinguations >stats and ones I got later. I don't think you can ignore clobbers. They are barriers for code motion. Richard. >Bootstrapped/regtested x86_64-linux. > >gcc/ChangeLog: > >2020-09-25 Jan Hubicka <hubicka@ucw.cz> > > * ipa-modref.c (analyze_stmt): Fix return value for gimple_clobber. > >diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c >index aa6929ff010..44b844b90db 100644 >--- a/gcc/ipa-modref.c >+++ b/gcc/ipa-modref.c >@@ -658,7 +658,7 @@ analyze_stmt (modref_summary *summary, gimple >*stmt, bool ipa) > { > /* There is no need to record clobbers. */ > if (gimple_clobber_p (stmt)) >- return false; >+ return true; > /* Analyze all loads and stores in STMT. */ > walk_stmt_load_store_ops (stmt, summary, > analyze_load, analyze_store);
> On September 26, 2020 12:04:24 AM GMT+02:00, Jan Hubicka <hubicka@ucw.cz> wrote: > >Hi, > >while adding check for gimple_clobber I reversed the return value > >so instead of ignoring the statement ipa-modref gives up. Fixed thus. > >This explains the drop between originally reported disambinguations > >stats and ones I got later. > > I don't think you can ignore clobbers. They are barriers for code motion. Hmm, you are right. In pure-const we do if ((ipa || cfun->after_inlining) && gimple_clobber_p (stmt)) that I think is safe. We don't do code motion in early passes but if we will and later inline, clobber will lead to wrong code. After inlining we will never see the clobber and thus ignoring it should be safe. Honza
> On September 26, 2020 12:04:24 AM GMT+02:00, Jan Hubicka <hubicka@ucw.cz> wrote: > >Hi, > >while adding check for gimple_clobber I reversed the return value > >so instead of ignoring the statement ipa-modref gives up. Fixed thus. > >This explains the drop between originally reported disambinguations > >stats and ones I got later. > > I don't think you can ignore clobbers. They are barriers for code motion. modref is (before and after patch) about 1.4% of the WPA time (2s). This is pretty the much cost of a single pass over the symbol table (other non-busy IPA passes takes about the same, ipa-comdat is fater with 0.7%). The iteration of dataflow happens only on non-trivial strongly connected components and at least for GCC alway terminates in 3 iterations (to trigger more one needs to function with many params with operation like shifting every param right. > > Richard. > > > >Bootstrapped/regtested x86_64-linux. > > > >gcc/ChangeLog: > > > >2020-09-25 Jan Hubicka <hubicka@ucw.cz> > > > > * ipa-modref.c (analyze_stmt): Fix return value for gimple_clobber. > > > >diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c > >index aa6929ff010..44b844b90db 100644 > >--- a/gcc/ipa-modref.c > >+++ b/gcc/ipa-modref.c > >@@ -658,7 +658,7 @@ analyze_stmt (modref_summary *summary, gimple > >*stmt, bool ipa) > > { > > /* There is no need to record clobbers. */ > > if (gimple_clobber_p (stmt)) > >- return false; > >+ return true; > > /* Analyze all loads and stores in STMT. */ > > walk_stmt_load_store_ops (stmt, summary, > > analyze_load, analyze_store); >
> On September 26, 2020 12:04:24 AM GMT+02:00, Jan Hubicka <hubicka@ucw.cz> wrote: > >Hi, > >while adding check for gimple_clobber I reversed the return value > >so instead of ignoring the statement ipa-modref gives up. Fixed thus. > >This explains the drop between originally reported disambinguations > >stats and ones I got later. > > I don't think you can ignore clobbers. They are barriers for code motion. Hi, this is fix I have installed after lto-bootstrapping/regtesting. The statistics for cc1plus are almost unchanged that is sort of expected given that I only measure late optimization by getting dump from LTO. Thank for pointing this out, it may have triggered a nasty wrong code bug :) Honza Alias oracle query stats: refs_may_alias_p: 63013346 disambiguations, 73204989 queries ref_maybe_used_by_call_p: 141350 disambiguations, 63909728 queries call_may_clobber_ref_p: 23597 disambiguations, 29430 queries nonoverlapping_component_refs_p: 0 disambiguations, 37763 queries nonoverlapping_refs_since_match_p: 19444 disambiguations, 55671 must overlaps, 75884 queries aliasing_component_refs_p: 54749 disambiguations, 753947 queries TBAA oracle: 24159888 disambiguations 56277876 queries 16064485 are in alias set 0 10340953 queries asked about the same object 125 queries asked about the same alias set 0 access volatile 3920604 are dependent in the DAG 1791821 are aritificially in conflict with void * Modref stats: modref use: 10444 disambiguations, 46994 queries modref clobber: 1421468 disambiguations, 1954304 queries 4907798 tbaa queries (2.511277 per modref query) 396785 base compares (0.203031 per modref query) PTA query stats: pt_solution_includes: 976073 disambiguations, 13607833 queries pt_solutions_intersect: 1026016 disambiguations, 13185678 queries * ipa-modref.c (analyze_stmt): Do not skip clobbers in early pass. * ipa-pure-const.c (analyze_stmt): Update comment. diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c index 73a7900883a..728c6c1523d 100644 --- a/gcc/ipa-modref.c +++ b/gcc/ipa-modref.c @@ -676,13 +676,16 @@ static bool analyze_stmt (modref_summary *summary, gimple *stmt, bool ipa, vec <gimple *> *recursive_calls) { - /* There is no need to record clobbers. */ - if (gimple_clobber_p (stmt)) + /* In general we can not ignore clobbers because they are barries for code + motion, however after inlining it is safe to do becuase local optimization + passes do not consider clobbers from other functions. + Similar logic is in ipa-pure-consts. */ + if ((ipa || cfun->after_inlining) && gimple_clobber_p (stmt)) return true; + /* Analyze all loads and stores in STMT. */ walk_stmt_load_store_ops (stmt, summary, analyze_load, analyze_store); - /* or call analyze_load_ipa, analyze_store_ipa */ switch (gimple_code (stmt)) { @@ -705,7 +708,7 @@ analyze_stmt (modref_summary *summary, gimple *stmt, bool ipa, } } -/* Analyze function F. IPA indicates whether we're running in tree mode (false) +/* Analyze function F. IPA indicates whether we're running in local mode (false) or the IPA mode (true). */ static void diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c index bdbccd010dc..1af3206056e 100644 --- a/gcc/ipa-pure-const.c +++ b/gcc/ipa-pure-const.c @@ -742,6 +742,8 @@ check_stmt (gimple_stmt_iterator *gsip, funct_state local, bool ipa) /* Do consider clobber as side effects before IPA, so we rather inline C++ destructors and keep clobber semantics than eliminate them. + Similar logic is in ipa-modref. + TODO: We may get smarter during early optimizations on these and let functions containing only clobbers to be optimized more. This is a common case of C++ destructors. */
Hi Honza, On Sat, 26 Sep 2020 at 22:03, Jan Hubicka <hubicka@ucw.cz> wrote: > > > On September 26, 2020 12:04:24 AM GMT+02:00, Jan Hubicka <hubicka@ucw.cz> wrote: > > >Hi, > > >while adding check for gimple_clobber I reversed the return value > > >so instead of ignoring the statement ipa-modref gives up. Fixed thus. > > >This explains the drop between originally reported disambinguations > > >stats and ones I got later. > > > > I don't think you can ignore clobbers. They are barriers for code motion. > > Hi, > this is fix I have installed after lto-bootstrapping/regtesting. > The statistics for cc1plus are almost unchanged that is sort of expected > given that I only measure late optimization by getting dump from LTO. > > Thank for pointing this out, it may have triggered a nasty wrong code > bug :) > > Honza > > Alias oracle query stats: > refs_may_alias_p: 63013346 disambiguations, 73204989 queries > ref_maybe_used_by_call_p: 141350 disambiguations, 63909728 queries > call_may_clobber_ref_p: 23597 disambiguations, 29430 queries > nonoverlapping_component_refs_p: 0 disambiguations, 37763 queries > nonoverlapping_refs_since_match_p: 19444 disambiguations, 55671 must overlaps, 75884 queries > aliasing_component_refs_p: 54749 disambiguations, 753947 queries > TBAA oracle: 24159888 disambiguations 56277876 queries > 16064485 are in alias set 0 > 10340953 queries asked about the same object > 125 queries asked about the same alias set > 0 access volatile > 3920604 are dependent in the DAG > 1791821 are aritificially in conflict with void * > > Modref stats: > modref use: 10444 disambiguations, 46994 queries > modref clobber: 1421468 disambiguations, 1954304 queries > 4907798 tbaa queries (2.511277 per modref query) > 396785 base compares (0.203031 per modref query) > > PTA query stats: > pt_solution_includes: 976073 disambiguations, 13607833 queries > pt_solutions_intersect: 1026016 disambiguations, 13185678 queries > > * ipa-modref.c (analyze_stmt): Do not skip clobbers in early pass. > * ipa-pure-const.c (analyze_stmt): Update comment. > After commit g:67a5c215940f4b21bac1aa489ce1f2fb3d52a53a (r11-3468), I see a failure in fortran on armeb-linux-gnueabihf, it's still present after r11-3491 (g4383c595ce5cc6ef6bcb45d2c9caf43002afbc4f): FAIL: gfortran.dg/PR94022.f90 -Os execution test with GCC configured with: --target armeb-none-linux-gnueabihf --with-mode arm --with-cpu cortex-a9 --with-fpu neon-fp16 Can you have a look? > diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c > index 73a7900883a..728c6c1523d 100644 > --- a/gcc/ipa-modref.c > +++ b/gcc/ipa-modref.c > @@ -676,13 +676,16 @@ static bool > analyze_stmt (modref_summary *summary, gimple *stmt, bool ipa, > vec <gimple *> *recursive_calls) > { > - /* There is no need to record clobbers. */ > - if (gimple_clobber_p (stmt)) > + /* In general we can not ignore clobbers because they are barries for code > + motion, however after inlining it is safe to do becuase local optimization > + passes do not consider clobbers from other functions. > + Similar logic is in ipa-pure-consts. */ > + if ((ipa || cfun->after_inlining) && gimple_clobber_p (stmt)) > return true; > + > /* Analyze all loads and stores in STMT. */ > walk_stmt_load_store_ops (stmt, summary, > analyze_load, analyze_store); > - /* or call analyze_load_ipa, analyze_store_ipa */ > > switch (gimple_code (stmt)) > { > @@ -705,7 +708,7 @@ analyze_stmt (modref_summary *summary, gimple *stmt, bool ipa, > } > } > > -/* Analyze function F. IPA indicates whether we're running in tree mode (false) > +/* Analyze function F. IPA indicates whether we're running in local mode (false) > or the IPA mode (true). */ > > static void > diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c > index bdbccd010dc..1af3206056e 100644 > --- a/gcc/ipa-pure-const.c > +++ b/gcc/ipa-pure-const.c > @@ -742,6 +742,8 @@ check_stmt (gimple_stmt_iterator *gsip, funct_state local, bool ipa) > /* Do consider clobber as side effects before IPA, so we rather inline > C++ destructors and keep clobber semantics than eliminate them. > > + Similar logic is in ipa-modref. > + > TODO: We may get smarter during early optimizations on these and let > functions containing only clobbers to be optimized more. This is a common > case of C++ destructors. */
> Hi Honza, > > > On Sat, 26 Sep 2020 at 22:03, Jan Hubicka <hubicka@ucw.cz> wrote: > > > > > On September 26, 2020 12:04:24 AM GMT+02:00, Jan Hubicka <hubicka@ucw.cz> wrote: > > > >Hi, > > > >while adding check for gimple_clobber I reversed the return value > > > >so instead of ignoring the statement ipa-modref gives up. Fixed thus. > > > >This explains the drop between originally reported disambinguations > > > >stats and ones I got later. > > > > > > I don't think you can ignore clobbers. They are barriers for code motion. > > > > Hi, > > this is fix I have installed after lto-bootstrapping/regtesting. > > The statistics for cc1plus are almost unchanged that is sort of expected > > given that I only measure late optimization by getting dump from LTO. > > > > Thank for pointing this out, it may have triggered a nasty wrong code > > bug :) > > > > Honza > > > > Alias oracle query stats: > > refs_may_alias_p: 63013346 disambiguations, 73204989 queries > > ref_maybe_used_by_call_p: 141350 disambiguations, 63909728 queries > > call_may_clobber_ref_p: 23597 disambiguations, 29430 queries > > nonoverlapping_component_refs_p: 0 disambiguations, 37763 queries > > nonoverlapping_refs_since_match_p: 19444 disambiguations, 55671 must overlaps, 75884 queries > > aliasing_component_refs_p: 54749 disambiguations, 753947 queries > > TBAA oracle: 24159888 disambiguations 56277876 queries > > 16064485 are in alias set 0 > > 10340953 queries asked about the same object > > 125 queries asked about the same alias set > > 0 access volatile > > 3920604 are dependent in the DAG > > 1791821 are aritificially in conflict with void * > > > > Modref stats: > > modref use: 10444 disambiguations, 46994 queries > > modref clobber: 1421468 disambiguations, 1954304 queries > > 4907798 tbaa queries (2.511277 per modref query) > > 396785 base compares (0.203031 per modref query) > > > > PTA query stats: > > pt_solution_includes: 976073 disambiguations, 13607833 queries > > pt_solutions_intersect: 1026016 disambiguations, 13185678 queries > > > > * ipa-modref.c (analyze_stmt): Do not skip clobbers in early pass. > > * ipa-pure-const.c (analyze_stmt): Update comment. > > > > After commit g:67a5c215940f4b21bac1aa489ce1f2fb3d52a53a (r11-3468), I > see a failure in fortran on armeb-linux-gnueabihf, > it's still present after r11-3491 (g4383c595ce5cc6ef6bcb45d2c9caf43002afbc4f): > FAIL: gfortran.dg/PR94022.f90 -Os execution test > with GCC configured with: > --target armeb-none-linux-gnueabihf > --with-mode arm > --with-cpu cortex-a9 > --with-fpu neon-fp16 > > Can you have a look? I will take a look. BTW on smaller testcases modref misoptimizations are generally not hard to debug. Compile with -fdump-tree-all-details and grep for modref. It prints when it disambiguates and then it is usually easy to find the problem (a reason why disambiguation is wrong). For fortran my guess is another frontend bug concering array descriptor as discussed here https://gcc.gnu.org/pipermail/gcc-patches/2020-September/554936.html It usually shows up as optimized out initialization of the array descriptor just before the call. Honza > > > diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c > > index 73a7900883a..728c6c1523d 100644 > > --- a/gcc/ipa-modref.c > > +++ b/gcc/ipa-modref.c > > @@ -676,13 +676,16 @@ static bool > > analyze_stmt (modref_summary *summary, gimple *stmt, bool ipa, > > vec <gimple *> *recursive_calls) > > { > > - /* There is no need to record clobbers. */ > > - if (gimple_clobber_p (stmt)) > > + /* In general we can not ignore clobbers because they are barries for code > > + motion, however after inlining it is safe to do becuase local optimization > > + passes do not consider clobbers from other functions. > > + Similar logic is in ipa-pure-consts. */ > > + if ((ipa || cfun->after_inlining) && gimple_clobber_p (stmt)) > > return true; > > + > > /* Analyze all loads and stores in STMT. */ > > walk_stmt_load_store_ops (stmt, summary, > > analyze_load, analyze_store); > > - /* or call analyze_load_ipa, analyze_store_ipa */ > > > > switch (gimple_code (stmt)) > > { > > @@ -705,7 +708,7 @@ analyze_stmt (modref_summary *summary, gimple *stmt, bool ipa, > > } > > } > > > > -/* Analyze function F. IPA indicates whether we're running in tree mode (false) > > +/* Analyze function F. IPA indicates whether we're running in local mode (false) > > or the IPA mode (true). */ > > > > static void > > diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c > > index bdbccd010dc..1af3206056e 100644 > > --- a/gcc/ipa-pure-const.c > > +++ b/gcc/ipa-pure-const.c > > @@ -742,6 +742,8 @@ check_stmt (gimple_stmt_iterator *gsip, funct_state local, bool ipa) > > /* Do consider clobber as side effects before IPA, so we rather inline > > C++ destructors and keep clobber semantics than eliminate them. > > > > + Similar logic is in ipa-modref. > > + > > TODO: We may get smarter during early optimizations on these and let > > functions containing only clobbers to be optimized more. This is a common > > case of C++ destructors. */
diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c index aa6929ff010..44b844b90db 100644 --- a/gcc/ipa-modref.c +++ b/gcc/ipa-modref.c @@ -658,7 +658,7 @@ analyze_stmt (modref_summary *summary, gimple *stmt, bool ipa) { /* There is no need to record clobbers. */ if (gimple_clobber_p (stmt)) - return false; + return true; /* Analyze all loads and stores in STMT. */ walk_stmt_load_store_ops (stmt, summary, analyze_load, analyze_store);