Re: [patch] backwards threader cleanups

[David Malcolm <dmalcolm at redhat dot com>]

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