Re: [patch] backwards threader cleanups
On 11/15/2017 08:06 AM, Pedro Alves wrote: On 11/15/2017 07:34 AM, Aldy Hernandez wrote: On 11/14/2017 02:38 PM, David Malcolm wrote: On Tue, 2017-11-14 at 14:08 -0500, Aldy Hernandez wrote: 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." Wow, I did not expect that order. Fixed. ... (Is this a self-assign from this->speed_p? should the "speed_p" param be renamed, e.g. to "speed_p_") Yes. Fixed. The convention also says: "When structs and/or classes have member functions, prefer to name data members with a leading m_". So in this case, the preference would be to rename this->speed_p to m_speed_p instead. Makes sense. It makes it easy to see what is a local variable and what is a class variable, as well as avoiding the ambiguity with the function argument. Thanks. I've adjusted the other data members of the class. Approved by Jeff down thread. Attaching the committed patch. Aldy gcc/ * hash-set.h (hash_set::empty): New. * tree-ssa-threadbackward.h: Delete. * 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..6fdbc9039f9 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 +{ + public: + void find_jump_threads_backwards (basic_block bb, bool speed_p); + private: + 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); + + /* Maximum number of BBs we are allowed to thread. */ + int m_max_threaded_paths; + /* Hash to keep track of seen bbs. */ + hash_set m_visited_bbs; + /* Current path we're analyzing. */ + auto_vec m_path; + /* Tracks if we have recursed through a loop PHI node. */ + bool m_seen_loop_phi; + /* Indicate that we could increase code size to improve the + code path. */ + bool m_speed_p; +}; /* Simple helper to get the last statement from BB, which is assumed to be a control statement. Return NULL if the last statement is @@ -61,14 +89,15 @@ get_gimple_control_stmt (basic_block bb) /* Return true if the CFG contains at least one path from START_BB to END_BB. When a path is found, record in PATH the blocks from - END_BB to START_BB. VISITED_BBS is used to make sure we don't fall - into an infinite loop. Bound the recursion to basic blocks - belonging to LOOP. */ + END_BB to START_BB. LOCAL_VISITED_BBS is used to make sure we + don't fall into an infinite loop. Bound the recursion to basic + blocks belonging to LOOP. */ static bool fsm_find_thread_path (basic_block start_bb, basic_block end_bb, vec , - hash_set _bbs, loop_p loop) + hash_set
Re: [patch] backwards threader cleanups
On 11/15/2017 12:34 AM, Aldy Hernandez wrote: > > > On 11/14/2017 02:38 PM, David Malcolm wrote: >> On Tue, 2017-11-14 at 14:08 -0500, Aldy Hernandez wrote: > >> 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." > > Wow, I did not expect that order. Fixed. > >> >> 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; > > As the comment says, these are per iteration, so I can't just put them > in a constructor. Perhaps I should say "per basic block" to make this > clearer. > >> >> (Is this a self-assign from this->speed_p? should the "speed_p" param >> be renamed, e.g. to "speed_p_") > > Yes. Fixed. > >> >>> 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) > > Per BB. I've lumped this with the block above that now says "per basic > block". > >> >> [...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? > > Done. > >> >> >>> @@ -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 > > Yes, very. Thanks! > > Updated patch attached. > Aldy > > curr.patch > > > 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. OK. jeff
Re: [patch] backwards threader cleanups
On 11/15/2017 07:34 AM, Aldy Hernandez wrote: > > > On 11/14/2017 02:38 PM, David Malcolm wrote: >> On Tue, 2017-11-14 at 14:08 -0500, Aldy Hernandez wrote: > >>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." > > Wow, I did not expect that order. Fixed. ... >> (Is this a self-assign from this->speed_p? should the "speed_p" param >> be renamed, e.g. to "speed_p_") > > Yes. Fixed. The convention also says: "When structs and/or classes have member functions, prefer to name data members with a leading m_". So in this case, the preference would be to rename this->speed_p to m_speed_p instead. Thanks, Pedro Alves
Re: [patch] backwards threader cleanups
On 11/14/2017 02:38 PM, David Malcolm wrote: On Tue, 2017-11-14 at 14:08 -0500, Aldy Hernandez wrote: 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." Wow, I did not expect that order. Fixed. 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; As the comment says, these are per iteration, so I can't just put them in a constructor. Perhaps I should say "per basic block" to make this clearer. (Is this a self-assign from this->speed_p? should the "speed_p" param be renamed, e.g. to "speed_p_") Yes. Fixed. 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) Per BB. I've lumped this with the block above that now says "per basic block". [...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? Done. @@ -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 Yes, very. Thanks! Updated patch attached. Aldy 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..5396f8e6761 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 +{ + public: + void find_jump_threads_backwards (basic_block bb, bool speed_p); + private: + 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
Re: [patch] backwards threader cleanups
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 visited_bbs; > + /* The current path we're analyzing. */ > + auto_vec 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
[patch] backwards threader cleanups
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? 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 visited_bbs; + /* The current path we're analyzing. */ + auto_vec 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); +}; /* Simple helper to get the last statement from BB, which is assumed to be a control statement. Return NULL if the last statement is @@ -61,14 +89,15 @@ get_gimple_control_stmt (basic_block bb) /* Return true if the CFG contains at least one path from START_BB to END_BB. When a path is found, record in PATH the blocks from - END_BB to START_BB. VISITED_BBS is used to make sure we don't fall - into an infinite loop. Bound the recursion to basic blocks - belonging to LOOP. */ + END_BB to START_BB. LOCAL_VISITED_BBS is used to make sure we + don't fall into an infinite loop. Bound the recursion to basic + blocks belonging to LOOP. */ static bool fsm_find_thread_path (basic_block start_bb, basic_block end_bb, vec , - hash_set _bbs, loop_p loop) + hash_set _visited_bbs, + loop_p loop) { if (loop != start_bb->loop_father) return false; @@ -79,12 +108,13 @@ fsm_find_thread_path (basic_block start_bb, basic_block end_bb, return true; } - if (!visited_bbs.add (start_bb)) + if (!local_visited_bbs.add (start_bb)) { edge e; edge_iterator ei; FOR_EACH_EDGE (e, ei, start_bb->succs) - if (fsm_find_thread_path (e->dest, end_bb, path, visited_bbs, loop)) + if