On Tue, 2017-11-14 at 14:08 -0500, Aldy Hernandez wrote:
> Howdy!
>
> For some upcoming work I need some pass local data that I don't want
> to
> be passing around as an argument. We have enough of those in the
> threader as it is. So I moved the current pass local data into its
> own
> class, and basically classified the entire pass, thus avoiding a lot
> of
> arguments.
>
> In doing this, I also noticed that not only was the prototype in the
> header file wrong, but it wasn't used anywhere. I have removed the
> useless header.
>
> The net result is less lines of code, even without taking into
> account
> the deleted header file.
>
> Oh yeah, we had no way of clearing a hash set. I've fixed this
> oversight :).
>
> Tested on x86-64 Linux.
>
> OK for trunk?
Some nitpicks below...
> gcc/
>
> * hash-set.h (hash_set::empty): New.
> * tree-ssa-threadbackward.h: Remove.
> * tree-ssa-threadbackward.c (class thread_jumps): New.
> Move max_threaded_paths into class.
> (fsm_find_thread_path): Remove arguments that are now in class.
> (profitable_jump_thread_path): Rename to...
> (thread_jumps::profitable_jump_thread_path): ...this.
> (convert_and_register_jump_thread_path): Rename to...
> (thread_jumps::convert_and_register_current_path): ...this.
> (check_subpath_and_update_thread_path): Rename to...
> (thread_jumps::check_subpath_and_update_thread_path): ...this.
> (register_jump_thread_path_if_profitable): Rename to...
> (thread_jumps::register_jump_thread_path_if_profitable): ...this.
> (handle_phi): Rename to...
> (thread_jumps::handle_phi): ...this.
> (handle_assignment): Rename to...
> (thread_jumps::handle_assignment): ...this.
> (fsm_find_control_statement_thread_paths): Rename to...
> (thread_jumps::fsm_find_control_statement_thread_paths): ...this.
> (find_jump_threads_backwards): Rename to...
> (thread_jumps::find_jump_threads_backwards): ...this.
> Initialize path local data.
> (pass_thread_jumps::execute): Call find_jump_threads_backwards
> from within thread_jumps class.
> (pass_early_thread_jumps::execute): Same.
>
> diff --git a/gcc/hash-set.h b/gcc/hash-set.h
> index d2247d39571..8ce796d1c48 100644
> --- a/gcc/hash-set.h
> +++ b/gcc/hash-set.h
> @@ -80,6 +80,10 @@ public:
>
> size_t elements () const { return m_table.elements (); }
>
> + /* Clear the hash table. */
> +
> + void empty () { m_table.empty (); }
> +
> class iterator
> {
> public:
> diff --git a/gcc/tree-ssa-threadbackward.c b/gcc/tree-ssa-threadbackward.c
> index 12bc80350f5..a1454f31bec 100644
> --- a/gcc/tree-ssa-threadbackward.c
> +++ b/gcc/tree-ssa-threadbackward.c
> @@ -38,7 +38,35 @@ along with GCC; see the file COPYING3. If not see
> #include "tree-inline.h"
> #include "tree-vectorizer.h"
>
> -static int max_threaded_paths;
> +class thread_jumps
> +{
> + private:
> + /* The maximum number of BB we are allowed to thread. */
> + int max_threaded_paths;
> + /* Hash to keep track of seen bbs. */
> + hash_set<basic_block> visited_bbs;
> + /* The current path we're analyzing. */
> + auto_vec<basic_block> path;
> + /* Tracks if we have recursed through a loop PHI node. */
> + bool seen_loop_phi;
> + /* Indicate that we could increase code size to improve the
> + code path. */
> + bool speed_p;
> +
> + edge profitable_jump_thread_path (basic_block bbi, tree name, tree arg,
> + bool *creates_irreducible_loop);
> + void convert_and_register_current_path (edge taken_edge);
> + void register_jump_thread_path_if_profitable (tree name, tree arg,
> + basic_block def_bb);
> + void handle_assignment (gimple *stmt, tree name, basic_block def_bb);
> + void handle_phi (gphi *phi, tree name, basic_block def_bb);
> + void fsm_find_control_statement_thread_paths (tree name);
> + bool check_subpath_and_update_thread_path (basic_block last_bb,
> + basic_block new_bb,
> + int *next_path_length);
> + public:
> + void find_jump_threads_backwards (basic_block bb, bool speed_p);
> +};
Nitpick:
https://gcc.gnu.org/codingconventions.html#Class_Form
says that:
"When defining a class, first [...]
declare all public member functions,
[...]
then declare all non-public member functions, and
then declare all non-public member variables."
Should the class have a ctor? I see in
thread_jumps::find_jump_threads_backwards
below that you have:
> + /* Initialize the pass local data that changes at each iteration. */
> + path.truncate (0);
> + path.safe_push (bb);
> + visited_bbs.empty ();
> + seen_loop_phi = false;
> + this->speed_p = speed_p;
(Is this a self-assign from this->speed_p? should the "speed_p" param
be renamed, e.g. to "speed_p_")
> max_threaded_paths = PARAM_VALUE (PARAM_MAX_FSM_THREAD_PATHS);
...but I'm not familiar enough with the code in question to be able
to know if it makes sense to move this initialization logic into a ctor
(i.e. is it per BB, or per CFG)
[...snip...]
> @@ -823,11 +810,12 @@ pass_thread_jumps::execute (function *fun)
> loop_optimizer_init (LOOPS_HAVE_PREHEADERS | LOOPS_HAVE_SIMPLE_LATCHES);
>
> /* Try to thread each block with more than one successor. */
> + thread_jumps pass;
> basic_block bb;
> FOR_EACH_BB_FN (bb, fun)
> {
> if (EDGE_COUNT (bb->succs) > 1)
> - find_jump_threads_backwards (bb, true);
> + pass.find_jump_threads_backwards (bb, true);
> }
> bool changed = thread_through_all_blocks (true);
Is it just me, or is "pass" a bit non-descript here?
How about "threader" or somesuch?
> @@ -883,11 +871,12 @@ pass_early_thread_jumps::execute (function *fun)
> loop_optimizer_init (AVOID_CFG_MODIFICATIONS);
>
> /* Try to thread each block with more than one successor. */
> + thread_jumps pass;
> basic_block bb;
> FOR_EACH_BB_FN (bb, fun)
> {
> if (EDGE_COUNT (bb->succs) > 1)
> - find_jump_threads_backwards (bb, false);
> + pass.find_jump_threads_backwards (bb, false);
> }
Similarly here
[...snip...]
Hope this is constructive
Dave
