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
