Re: [PATCH] Make path_range_query standalone and add reset_path.
On Thu, Aug 18, 2022 at 1:10 PM Aldy Hernandez wrote:
>
> On Thu, Aug 18, 2022, 11:34 Richard Biener wrote:
> >
> > On Thu, Aug 18, 2022 at 9:58 AM Aldy Hernandez wrote:
> > >
> > > FWIW, here is a rebased version with the latest trunk.
> > >
> > > There are no regressions, or differences in threading counts, and the
> > > performance change is negligible.
> >
> > +{
> > + mark_dfs_back_edges ();
> > +
> > + if (flag_checking)
> > + verify_marked_backedges (fun);
> >
> > that looks redundant.
>
> Do you suggest somewhere else, or should we nuke
> verify_marked_backedges altogether since that's its only use?
Not sure. I'd leave the function around in any case.
> >
> > Otherwise looks good - it might be possible to get rid of the reset_path use
> > as well?
>
>
> That's what I mentioned wrt DOM threading. There's a 14% degradation
> from not reusing the path_range_query in the forward threader because
> it really abuses the query to try to simplify its statements. So I've
> left reset_path for it to use, but we can get rid of it when we nuke
> the old threader.
Ah, I see.
> Aldy
>
>
> Aldy
>
> >
> > Richard.
> >
> > > Aldy
> > >
> > > On Wed, Aug 17, 2022 at 1:59 PM Aldy Hernandez wrote:
> > > >
> > > > These are a bunch of cleanups inspired by Richi's suggestion of making
> > > > path_range_query standalone, instead of having to call
> > > > compute_ranges() for each path.
> > > >
> > > > I've made the ranger need explicit, and moved the responsibility for
> > > > its creation to the caller.
> > > >
> > > > I've also investigated and documented why the forward threader needs its
> > > > own compute exit dependencies variant. I can't wait for it to go away
> > > > :-/.
> > > >
> > > > I've also added constructors that take a path and dependencies, and
> > > > made compute_ranges() private. Unfortunately, reinstantiating
> > > > path_range_query in the forward threader caused a 14% performance
> > > > regression in DOM, because the old threader calls it over and over on
> > > > the same path to simplify statements (some of which not even in the
> > > > IL, but that's old news).
> > > >
> > > > In the meantime, I've left the ability to reset a path, but this time
> > > > appropriately called reset_path().
> > > >
> > > > I've moved the verify_marked_backedges call to the threader. Is this
> > > > ok?
> > > >
> > > > Interestingly, comparing the thread counts for the patch yielded more
> > > > threads. I narrowed this down to a bug in the path oracle reset code
> > > > that's not cleaning things up as expected. I'll fix that before
> > > > committing this, but figured I'd post for comments in the meantime.
> > > >
> > > > Thoughts?
> > > >
> > > > gcc/ChangeLog:
> > > >
> > > > * gimple-range-path.cc (path_range_query::path_range_query): Add
> > > > various constructors to take a path.
> > > > (path_range_query::~path_range_query): Remove m_alloced_ranger.
> > > > (path_range_query::range_on_path_entry): Adjust for m_ranger
> > > > being
> > > > a reference.
> > > > (path_range_query::set_path): Rename to...
> > > > (path_range_query::reset_path): ...this and call compute_ranges.
> > > > (path_range_query::ssa_range_in_phi): Adjust for m_ranger
> > > > reference.
> > > > (path_range_query::range_defined_in_block): Same.
> > > > (path_range_query::compute_ranges_in_block): Same.
> > > > (path_range_query::adjust_for_non_null_uses): Same.
> > > > (path_range_query::compute_exit_dependencies): Use m_path
> > > > instead
> > > > of argument.
> > > > (path_range_query::compute_ranges): Remove path argument.
> > > > (path_range_query::range_of_stmt): Adjust for m_ranger
> > > > reference.
> > > > (path_range_query::compute_outgoing_relations): Same.
> > > > * gimple-range-path.h (class path_range_query): Add various
> > > > constructors.
> > > > Make compute_ranges and compute_exit_dependencies private.
> > > > Rename set_path to reset_path.
> > > > Make m_ranger a reference.
> > > > Remove m_alloced_ranger.
> > > > * tree-ssa-dom.cc (pass_dominator::execute): Adjust constructor
> > > > to
> > > > path_range_query.
> > > > * tree-ssa-loop-ch.cc (entry_loop_condition_is_static): Take a
> > > > ranger and instantiate a new path_range_query every time.
> > > > (ch_base::copy_headers): Pass ranger instead of
> > > > path_range_query.
> > > > * tree-ssa-threadbackward.cc (class back_threader): Remove
> > > > m_solver.
> > > > (back_threader::~back_threader): Remove m_solver.
> > > > (back_threader::find_taken_edge_switch): Adjust for m_ranger
> > > > reference.
> > > > (back_threader::find_taken_edge_cond): Same.
> > > > (back_threader::dump): Remove m_solver.
> > > > (back_threader::back_threader): Move v
Re: [PATCH] Make path_range_query standalone and add reset_path.
On Thu, Aug 18, 2022, 11:34 Richard Biener wrote:
>
> On Thu, Aug 18, 2022 at 9:58 AM Aldy Hernandez wrote:
> >
> > FWIW, here is a rebased version with the latest trunk.
> >
> > There are no regressions, or differences in threading counts, and the
> > performance change is negligible.
>
> +{
> + mark_dfs_back_edges ();
> +
> + if (flag_checking)
> + verify_marked_backedges (fun);
>
> that looks redundant.
Do you suggest somewhere else, or should we nuke
verify_marked_backedges altogether since that's its only use?
>
> Otherwise looks good - it might be possible to get rid of the reset_path use
> as well?
That's what I mentioned wrt DOM threading. There's a 14% degradation
from not reusing the path_range_query in the forward threader because
it really abuses the query to try to simplify its statements. So I've
left reset_path for it to use, but we can get rid of it when we nuke
the old threader.
Aldy
Aldy
>
> Richard.
>
> > Aldy
> >
> > On Wed, Aug 17, 2022 at 1:59 PM Aldy Hernandez wrote:
> > >
> > > These are a bunch of cleanups inspired by Richi's suggestion of making
> > > path_range_query standalone, instead of having to call
> > > compute_ranges() for each path.
> > >
> > > I've made the ranger need explicit, and moved the responsibility for
> > > its creation to the caller.
> > >
> > > I've also investigated and documented why the forward threader needs its
> > > own compute exit dependencies variant. I can't wait for it to go away
> > > :-/.
> > >
> > > I've also added constructors that take a path and dependencies, and
> > > made compute_ranges() private. Unfortunately, reinstantiating
> > > path_range_query in the forward threader caused a 14% performance
> > > regression in DOM, because the old threader calls it over and over on
> > > the same path to simplify statements (some of which not even in the
> > > IL, but that's old news).
> > >
> > > In the meantime, I've left the ability to reset a path, but this time
> > > appropriately called reset_path().
> > >
> > > I've moved the verify_marked_backedges call to the threader. Is this
> > > ok?
> > >
> > > Interestingly, comparing the thread counts for the patch yielded more
> > > threads. I narrowed this down to a bug in the path oracle reset code
> > > that's not cleaning things up as expected. I'll fix that before
> > > committing this, but figured I'd post for comments in the meantime.
> > >
> > > Thoughts?
> > >
> > > gcc/ChangeLog:
> > >
> > > * gimple-range-path.cc (path_range_query::path_range_query): Add
> > > various constructors to take a path.
> > > (path_range_query::~path_range_query): Remove m_alloced_ranger.
> > > (path_range_query::range_on_path_entry): Adjust for m_ranger being
> > > a reference.
> > > (path_range_query::set_path): Rename to...
> > > (path_range_query::reset_path): ...this and call compute_ranges.
> > > (path_range_query::ssa_range_in_phi): Adjust for m_ranger
> > > reference.
> > > (path_range_query::range_defined_in_block): Same.
> > > (path_range_query::compute_ranges_in_block): Same.
> > > (path_range_query::adjust_for_non_null_uses): Same.
> > > (path_range_query::compute_exit_dependencies): Use m_path instead
> > > of argument.
> > > (path_range_query::compute_ranges): Remove path argument.
> > > (path_range_query::range_of_stmt): Adjust for m_ranger reference.
> > > (path_range_query::compute_outgoing_relations): Same.
> > > * gimple-range-path.h (class path_range_query): Add various
> > > constructors.
> > > Make compute_ranges and compute_exit_dependencies private.
> > > Rename set_path to reset_path.
> > > Make m_ranger a reference.
> > > Remove m_alloced_ranger.
> > > * tree-ssa-dom.cc (pass_dominator::execute): Adjust constructor to
> > > path_range_query.
> > > * tree-ssa-loop-ch.cc (entry_loop_condition_is_static): Take a
> > > ranger and instantiate a new path_range_query every time.
> > > (ch_base::copy_headers): Pass ranger instead of path_range_query.
> > > * tree-ssa-threadbackward.cc (class back_threader): Remove
> > > m_solver.
> > > (back_threader::~back_threader): Remove m_solver.
> > > (back_threader::find_taken_edge_switch): Adjust for m_ranger
> > > reference.
> > > (back_threader::find_taken_edge_cond): Same.
> > > (back_threader::dump): Remove m_solver.
> > > (back_threader::back_threader): Move verify_marked_backedges
> > > here from the path_range_query constructor.
> > > * tree-ssa-threadedge.cc (hybrid_jt_simplifier::simplify): Move
> > > some code from compute_ranges_from_state here.
> > > (hybrid_jt_simplifier::compute_ranges_from_state): Rename...
> > > (hybrid_jt_simplifier::compute_exit_dependencies): ...to this.
> > >
Re: [PATCH] Make path_range_query standalone and add reset_path.
On Thu, Aug 18, 2022 at 9:58 AM Aldy Hernandez wrote:
>
> FWIW, here is a rebased version with the latest trunk.
>
> There are no regressions, or differences in threading counts, and the
> performance change is negligible.
+{
+ mark_dfs_back_edges ();
+
+ if (flag_checking)
+ verify_marked_backedges (fun);
that looks redundant.
Otherwise looks good - it might be possible to get rid of the reset_path use
as well?
Richard.
> Aldy
>
> On Wed, Aug 17, 2022 at 1:59 PM Aldy Hernandez wrote:
> >
> > These are a bunch of cleanups inspired by Richi's suggestion of making
> > path_range_query standalone, instead of having to call
> > compute_ranges() for each path.
> >
> > I've made the ranger need explicit, and moved the responsibility for
> > its creation to the caller.
> >
> > I've also investigated and documented why the forward threader needs its
> > own compute exit dependencies variant. I can't wait for it to go away
> > :-/.
> >
> > I've also added constructors that take a path and dependencies, and
> > made compute_ranges() private. Unfortunately, reinstantiating
> > path_range_query in the forward threader caused a 14% performance
> > regression in DOM, because the old threader calls it over and over on
> > the same path to simplify statements (some of which not even in the
> > IL, but that's old news).
> >
> > In the meantime, I've left the ability to reset a path, but this time
> > appropriately called reset_path().
> >
> > I've moved the verify_marked_backedges call to the threader. Is this
> > ok?
> >
> > Interestingly, comparing the thread counts for the patch yielded more
> > threads. I narrowed this down to a bug in the path oracle reset code
> > that's not cleaning things up as expected. I'll fix that before
> > committing this, but figured I'd post for comments in the meantime.
> >
> > Thoughts?
> >
> > gcc/ChangeLog:
> >
> > * gimple-range-path.cc (path_range_query::path_range_query): Add
> > various constructors to take a path.
> > (path_range_query::~path_range_query): Remove m_alloced_ranger.
> > (path_range_query::range_on_path_entry): Adjust for m_ranger being
> > a reference.
> > (path_range_query::set_path): Rename to...
> > (path_range_query::reset_path): ...this and call compute_ranges.
> > (path_range_query::ssa_range_in_phi): Adjust for m_ranger
> > reference.
> > (path_range_query::range_defined_in_block): Same.
> > (path_range_query::compute_ranges_in_block): Same.
> > (path_range_query::adjust_for_non_null_uses): Same.
> > (path_range_query::compute_exit_dependencies): Use m_path instead
> > of argument.
> > (path_range_query::compute_ranges): Remove path argument.
> > (path_range_query::range_of_stmt): Adjust for m_ranger reference.
> > (path_range_query::compute_outgoing_relations): Same.
> > * gimple-range-path.h (class path_range_query): Add various
> > constructors.
> > Make compute_ranges and compute_exit_dependencies private.
> > Rename set_path to reset_path.
> > Make m_ranger a reference.
> > Remove m_alloced_ranger.
> > * tree-ssa-dom.cc (pass_dominator::execute): Adjust constructor to
> > path_range_query.
> > * tree-ssa-loop-ch.cc (entry_loop_condition_is_static): Take a
> > ranger and instantiate a new path_range_query every time.
> > (ch_base::copy_headers): Pass ranger instead of path_range_query.
> > * tree-ssa-threadbackward.cc (class back_threader): Remove m_solver.
> > (back_threader::~back_threader): Remove m_solver.
> > (back_threader::find_taken_edge_switch): Adjust for m_ranger
> > reference.
> > (back_threader::find_taken_edge_cond): Same.
> > (back_threader::dump): Remove m_solver.
> > (back_threader::back_threader): Move verify_marked_backedges
> > here from the path_range_query constructor.
> > * tree-ssa-threadedge.cc (hybrid_jt_simplifier::simplify): Move
> > some code from compute_ranges_from_state here.
> > (hybrid_jt_simplifier::compute_ranges_from_state): Rename...
> > (hybrid_jt_simplifier::compute_exit_dependencies): ...to this.
> > * tree-ssa-threadedge.h (class hybrid_jt_simplifier): Rename
> > compute_ranges_from_state to compute_exit_dependencies.
> > Remove m_path.
> > ---
> > gcc/gimple-range-path.cc | 112 +
> > gcc/gimple-range-path.h| 22 +++
> > gcc/tree-ssa-dom.cc| 2 +-
> > gcc/tree-ssa-loop-ch.cc| 12 ++--
> > gcc/tree-ssa-threadbackward.cc | 27
> > gcc/tree-ssa-threadedge.cc | 30 +
> > gcc/tree-ssa-threadedge.h | 5 +-
> > 7 files changed, 111 insertions(+), 99 deletions(-)
> >
> > diff --git a/gcc/gimple-range-path.cc b/gcc/gimple-range-path.cc
> > index c99d7
Re: [PATCH] Make path_range_query standalone and add reset_path.
FWIW, here is a rebased version with the latest trunk. There are no regressions, or differences in threading counts, and the performance change is negligible. Aldy On Wed, Aug 17, 2022 at 1:59 PM Aldy Hernandez wrote: > > These are a bunch of cleanups inspired by Richi's suggestion of making > path_range_query standalone, instead of having to call > compute_ranges() for each path. > > I've made the ranger need explicit, and moved the responsibility for > its creation to the caller. > > I've also investigated and documented why the forward threader needs its > own compute exit dependencies variant. I can't wait for it to go away > :-/. > > I've also added constructors that take a path and dependencies, and > made compute_ranges() private. Unfortunately, reinstantiating > path_range_query in the forward threader caused a 14% performance > regression in DOM, because the old threader calls it over and over on > the same path to simplify statements (some of which not even in the > IL, but that's old news). > > In the meantime, I've left the ability to reset a path, but this time > appropriately called reset_path(). > > I've moved the verify_marked_backedges call to the threader. Is this > ok? > > Interestingly, comparing the thread counts for the patch yielded more > threads. I narrowed this down to a bug in the path oracle reset code > that's not cleaning things up as expected. I'll fix that before > committing this, but figured I'd post for comments in the meantime. > > Thoughts? > > gcc/ChangeLog: > > * gimple-range-path.cc (path_range_query::path_range_query): Add > various constructors to take a path. > (path_range_query::~path_range_query): Remove m_alloced_ranger. > (path_range_query::range_on_path_entry): Adjust for m_ranger being > a reference. > (path_range_query::set_path): Rename to... > (path_range_query::reset_path): ...this and call compute_ranges. > (path_range_query::ssa_range_in_phi): Adjust for m_ranger > reference. > (path_range_query::range_defined_in_block): Same. > (path_range_query::compute_ranges_in_block): Same. > (path_range_query::adjust_for_non_null_uses): Same. > (path_range_query::compute_exit_dependencies): Use m_path instead > of argument. > (path_range_query::compute_ranges): Remove path argument. > (path_range_query::range_of_stmt): Adjust for m_ranger reference. > (path_range_query::compute_outgoing_relations): Same. > * gimple-range-path.h (class path_range_query): Add various > constructors. > Make compute_ranges and compute_exit_dependencies private. > Rename set_path to reset_path. > Make m_ranger a reference. > Remove m_alloced_ranger. > * tree-ssa-dom.cc (pass_dominator::execute): Adjust constructor to > path_range_query. > * tree-ssa-loop-ch.cc (entry_loop_condition_is_static): Take a > ranger and instantiate a new path_range_query every time. > (ch_base::copy_headers): Pass ranger instead of path_range_query. > * tree-ssa-threadbackward.cc (class back_threader): Remove m_solver. > (back_threader::~back_threader): Remove m_solver. > (back_threader::find_taken_edge_switch): Adjust for m_ranger > reference. > (back_threader::find_taken_edge_cond): Same. > (back_threader::dump): Remove m_solver. > (back_threader::back_threader): Move verify_marked_backedges > here from the path_range_query constructor. > * tree-ssa-threadedge.cc (hybrid_jt_simplifier::simplify): Move > some code from compute_ranges_from_state here. > (hybrid_jt_simplifier::compute_ranges_from_state): Rename... > (hybrid_jt_simplifier::compute_exit_dependencies): ...to this. > * tree-ssa-threadedge.h (class hybrid_jt_simplifier): Rename > compute_ranges_from_state to compute_exit_dependencies. > Remove m_path. > --- > gcc/gimple-range-path.cc | 112 + > gcc/gimple-range-path.h| 22 +++ > gcc/tree-ssa-dom.cc| 2 +- > gcc/tree-ssa-loop-ch.cc| 12 ++-- > gcc/tree-ssa-threadbackward.cc | 27 > gcc/tree-ssa-threadedge.cc | 30 + > gcc/tree-ssa-threadedge.h | 5 +- > 7 files changed, 111 insertions(+), 99 deletions(-) > > diff --git a/gcc/gimple-range-path.cc b/gcc/gimple-range-path.cc > index c99d77dd340..d37605f4539 100644 > --- a/gcc/gimple-range-path.cc > +++ b/gcc/gimple-range-path.cc > @@ -36,33 +36,52 @@ along with GCC; see the file COPYING3. If not see > // Internal construct to help facilitate debugging of solver. > #define DEBUG_SOLVER (dump_file && (param_threader_debug == > THREADER_DEBUG_ALL)) > > -path_range_query::path_range_query (bool resolve, gimple_ranger *ranger) > +path_range_query::path_range_query (gimple_ranger &ranger, > +
