This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] manage dom-walk_data initialization and finalization with constructors and destructors


Hello,

On Tue, 17 Sep 2013, Jeff Law wrote:

> I put the attached patch through a bootstrap and regression test cycle on
> x86_64-unknown-linux-gnu.  As expected no regressions.  Installed onto the
> trunk.
> 
> Thanks Trevor!

You missed one comment style nit, and some ChangeLog entry nits.

I'm irritated by the member name uglification (e.g. equiv_stack_ with 
trailing underscore).  I know that's a certain style to mark private 
members, but I think it's a bad style (like prefixing variable names with 
their type), and before it sets a precedent in GCCs c++ coding style I'd 
like this to be changed, like in the below.

I'd also like us to not use member privatization in our classes, but 
that's not in the patch, but if we could agree on that it would be nice.

Regstrapped on x86-64-linux, okay?


Ciao,
Michael.

	* domwalk.h (dom_walker::dom_direction_): Rename to dom_direction.
	* domwalk.c (dom_walker::walk): Adjust.
	* graphite-sese-to-poly.c (sese_dom_walker::region_): Rename to
	region.
	(sese_dom_walker::sese_dom_walker, sese_dom_walker::~sese_dom_walker,
	sese_dom_walker::before_dom_children,
	sese_dom_walker::after_dom_children): Adjust.
	* tree-into-ssa.c (mark_def_dom_walker::kills_): Rename to
	kills.
	(mark_def_dom_walker::mark_def_dom_walker,
	mark_def_dom_walker::before_dom_children): Adjust.
	* tree-ssa-dom.c (dom_opt_dom_walker::dummy_cond_): Rename to
	dummy_cond.
	(dom_opt_dom_walker::thread_across_edge): Adjust.
	* tree-ssa-loop-im.c (move_computations_dom_walker::todo_): Rename
	to todo.
	(move_computations_dom_walker::before_dom,
	move_computations): Adjust.
	* tree-ssa-phiopt.c (nontrapping_dom_walker::nontrapping_): Rename
	to nontrapping.
	(nontrapping_dom_walker::before_dom_child): Adjust.
	* tree-ssa-uncprop.c (uncprop_dom_walker::equiv_stack_): Rename
	to equiv_stack.
	(uncprop_dom_walker::uncprop_dom_walker,
	uncprop_dom_walker::~uncprop_dom_walker,
	uncprop_dom_walker::after_dom_children): Adjust.

Index: domwalk.h
===================================================================
--- domwalk.h	(revision 202698)
+++ domwalk.h	(working copy)
@@ -21,16 +21,14 @@ along with GCC; see the file COPYING3.
 #ifndef GCC_DOM_WALK_H
 #define GCC_DOM_WALK_H
 
-/**
- * This is the main class for the dominator walker. It is expected that
- * consumers will have a custom class inheriting from it, which will over ride
- * at least one of before_dom_children and after_dom_children to implement the
- * custom behavior.
- */
+/* This is the main class for the dominator walker.  It is expected that
+   consumers will have a custom class inheriting from it, which will over ride
+   at least one of before_dom_children and after_dom_children to implement the
+   custom behavior.  */
 class dom_walker
 {
 public:
-  dom_walker (cdi_direction direction) : dom_direction_ (direction) {}
+  dom_walker (cdi_direction direction) : dom_direction (direction) {}
 
   /* Walk the dominator tree.  */
   void walk (basic_block);
@@ -46,7 +44,7 @@ private:
      if it is set to CDI_DOMINATORS, then we walk the dominator tree,
      if it is set to CDI_POST_DOMINATORS, then we walk the post
      dominator tree.  */
-  const ENUM_BITFIELD (cdi_direction) dom_direction_ : 2;
+  const ENUM_BITFIELD (cdi_direction) dom_direction : 2;
 };
 
 #endif
Index: domwalk.c
===================================================================
--- domwalk.c	(revision 202698)
+++ domwalk.c	(working copy)
@@ -154,7 +154,7 @@ dom_walker::walk (basic_block bb)
   int sp = 0;
   int *postorder, postorder_num;
 
-  if (dom_direction_ == CDI_DOMINATORS)
+  if (dom_direction == CDI_DOMINATORS)
     {
       postorder = XNEWVEC (int, n_basic_blocks);
       postorder_num = inverted_post_order_compute (postorder);
@@ -181,10 +181,10 @@ dom_walker::walk (basic_block bb)
 	  worklist[sp++] = NULL;
 
 	  int saved_sp = sp;
-	  for (dest = first_dom_son (dom_direction_, bb);
-	       dest; dest = next_dom_son (dom_direction_, dest))
+	  for (dest = first_dom_son (dom_direction, bb);
+	       dest; dest = next_dom_son (dom_direction, dest))
 	    worklist[sp++] = dest;
-	  if (dom_direction_ == CDI_DOMINATORS)
+	  if (dom_direction == CDI_DOMINATORS)
 	    switch (sp - saved_sp)
 	      {
 	      case 0:
@@ -210,7 +210,7 @@ dom_walker::walk (basic_block bb)
       else
 	break;
     }
-  if (dom_direction_ == CDI_DOMINATORS)
+  if (dom_direction == CDI_DOMINATORS)
     {
       free (bb_postorder);
       bb_postorder = NULL;
Index: graphite-sese-to-poly.c
===================================================================
--- graphite-sese-to-poly.c	(revision 202698)
+++ graphite-sese-to-poly.c	(working copy)
@@ -1226,21 +1226,21 @@ public:
   virtual void after_dom_children (basic_block);
 
 private:
-  vec<gimple> conditions_, cases_;
-  sese region_;
+  vec<gimple> conditions, cases;
+  sese region;
 };
 
 sese_dom_walker::sese_dom_walker (cdi_direction direction, sese region)
-  : dom_walker (direction), region_ (region)
+  : dom_walker (direction), region (region)
 {
-  conditions_.create (3);
-  cases_.create (3);
+  conditions.create (3);
+  cases.create (3);
 }
 
 sese_dom_walker::~sese_dom_walker ()
 {
-  conditions_.release ();
-  cases_.release ();
+  conditions.release ();
+  cases.release ();
 }
 
 /* Call-back for dom_walk executed before visiting the dominated
@@ -1252,7 +1252,7 @@ sese_dom_walker::before_dom_children (ba
   gimple_bb_p gbb;
   gimple stmt;
 
-  if (!bb_in_sese_p (bb, region_))
+  if (!bb_in_sese_p (bb, region))
     return;
 
   stmt = single_pred_cond_non_loop_exit (bb);
@@ -1261,20 +1261,20 @@ sese_dom_walker::before_dom_children (ba
     {
       edge e = single_pred_edge (bb);
 
-      conditions_.safe_push (stmt);
+      conditions.safe_push (stmt);
 
       if (e->flags & EDGE_TRUE_VALUE)
-	cases_.safe_push (stmt);
+	cases.safe_push (stmt);
       else
-	cases_.safe_push (NULL);
+	cases.safe_push (NULL);
     }
 
   gbb = gbb_from_bb (bb);
 
   if (gbb)
     {
-      GBB_CONDITIONS (gbb) = conditions_.copy ();
-      GBB_CONDITION_CASES (gbb) = cases_.copy ();
+      GBB_CONDITIONS (gbb) = conditions.copy ();
+      GBB_CONDITION_CASES (gbb) = cases.copy ();
     }
 }
 
@@ -1284,13 +1284,13 @@ sese_dom_walker::before_dom_children (ba
 void
 sese_dom_walker::after_dom_children (basic_block bb)
 {
-  if (!bb_in_sese_p (bb, region_))
+  if (!bb_in_sese_p (bb, region))
     return;
 
   if (single_pred_cond_non_loop_exit (bb))
     {
-      conditions_.pop ();
-      cases_.pop ();
+      conditions.pop ();
+      cases.pop ();
     }
 }
 
Index: tree-into-ssa.c
===================================================================
--- tree-into-ssa.c	(revision 202698)
+++ tree-into-ssa.c	(working copy)
@@ -2075,7 +2075,8 @@ rewrite_update_phi_arguments (basic_bloc
 class rewrite_update_dom_walker : public dom_walker
 {
 public:
-  rewrite_update_dom_walker (cdi_direction direction) : dom_walker (direction) {}
+  rewrite_update_dom_walker (cdi_direction direction) : dom_walker (direction)
+  {}
 
   virtual void before_dom_children (basic_block);
   virtual void after_dom_children (basic_block);
@@ -2235,17 +2236,17 @@ private:
   /* Notice that this bitmap is indexed using variable UIDs, so it must be
      large enough to accommodate all the variables referenced in the
      function, not just the ones we are renaming.  */
-  bitmap kills_;
+  bitmap kills;
 };
 
 mark_def_dom_walker::mark_def_dom_walker (cdi_direction direction)
-  : dom_walker (direction), kills_ (BITMAP_ALLOC (NULL))
+  : dom_walker (direction), kills (BITMAP_ALLOC (NULL))
 {
 }
 
 mark_def_dom_walker::~mark_def_dom_walker ()
 {
-  BITMAP_FREE (kills_);
+  BITMAP_FREE (kills);
 }
 
 /* Block processing routine for mark_def_sites.  Clear the KILLS bitmap
@@ -2256,9 +2257,9 @@ mark_def_dom_walker::before_dom_children
 {
   gimple_stmt_iterator gsi;
 
-  bitmap_clear (kills_);
+  bitmap_clear (kills);
   for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
-    mark_def_sites (bb, gsi_stmt (gsi), kills_);
+    mark_def_sites (bb, gsi_stmt (gsi), kills);
 }
 
 /* Initialize internal data needed during renaming.  */
Index: tree-ssa-dom.c
===================================================================
--- tree-ssa-dom.c	(revision 202698)
+++ tree-ssa-dom.c	(working copy)
@@ -774,7 +774,7 @@ class dom_opt_dom_walker : public dom_wa
 {
 public:
   dom_opt_dom_walker (cdi_direction direction)
-    : dom_walker (direction), dummy_cond_ (NULL) {}
+    : dom_walker (direction), dummy_cond (NULL) {}
 
   virtual void before_dom_children (basic_block);
   virtual void after_dom_children (basic_block);
@@ -782,7 +782,7 @@ public:
 private:
   void thread_across_edge (edge);
 
-  gimple dummy_cond_;
+  gimple dummy_cond;
 };
 
 /* Jump threading, redundancy elimination and const/copy propagation.
@@ -1077,13 +1077,13 @@ simplify_stmt_for_jump_threading (gimple
 void
 dom_opt_dom_walker::thread_across_edge (edge e)
 {
-  if (! dummy_cond_)
-    dummy_cond_ =
+  if (! dummy_cond)
+    dummy_cond =
         gimple_build_cond (NE_EXPR,
                            integer_zero_node, integer_zero_node,
                            NULL, NULL);
 
-  ::thread_across_edge (dummy_cond_, e, false,
+  ::thread_across_edge (dummy_cond, e, false,
 		        &const_and_copies_stack,
 		        simplify_stmt_for_jump_threading);
 }
Index: tree-ssa-loop-im.c
===================================================================
--- tree-ssa-loop-im.c	(revision 202698)
+++ tree-ssa-loop-im.c	(working copy)
@@ -1192,11 +1192,11 @@ class move_computations_dom_walker : pub
 {
 public:
   move_computations_dom_walker (cdi_direction direction)
-    : dom_walker (direction), todo_ (0) {}
+    : dom_walker (direction), todo (0) {}
 
   virtual void before_dom_children (basic_block);
 
-  unsigned int todo_;
+  unsigned int todo;
 };
 
 /* Hoist the statements in basic block BB out of the loops prescribed by
@@ -1268,7 +1268,7 @@ move_computations_dom_walker::before_dom
 						   gimple_phi_result (stmt),
 						   t, arg0, arg1);
 	  SSA_NAME_DEF_STMT (gimple_phi_result (stmt)) = new_stmt;
-	  todo_ |= TODO_cleanup_cfg;
+	  todo |= TODO_cleanup_cfg;
 	}
       gsi_insert_on_edge (loop_preheader_edge (level), new_stmt);
       remove_phi_node (&bsi, false);
@@ -1346,7 +1346,7 @@ move_computations (void)
   if (need_ssa_update_p (cfun))
     rewrite_into_loop_closed_ssa (NULL, TODO_update_ssa);
 
-  return walker.todo_;
+  return walker.todo;
 }
 
 /* Checks whether the statement defining variable *INDEX can be hoisted
Index: tree-ssa-phiopt.c
===================================================================
--- tree-ssa-phiopt.c	(revision 202698)
+++ tree-ssa-phiopt.c	(working copy)
@@ -1379,13 +1379,13 @@ class nontrapping_dom_walker : public do
 {
 public:
   nontrapping_dom_walker (cdi_direction direction, pointer_set_t *ps)
-    : dom_walker (direction), nontrapping_ (ps) {}
+    : dom_walker (direction), nontrapping (ps) {}
 
   virtual void before_dom_children (basic_block);
   virtual void after_dom_children (basic_block);
 
 private:
-  pointer_set_t *nontrapping_;
+  pointer_set_t *nontrapping;
 };
 
 /* Called by walk_dominator_tree, when entering the block BB.  */
@@ -1416,8 +1416,8 @@ nontrapping_dom_walker::before_dom_child
 	nt_call_phase++;
       else if (gimple_assign_single_p (stmt) && !gimple_has_volatile_ops (stmt))
 	{
-	  add_or_mark_expr (bb, gimple_assign_lhs (stmt), nontrapping_, true);
-	  add_or_mark_expr (bb, gimple_assign_rhs1 (stmt), nontrapping_, false);
+	  add_or_mark_expr (bb, gimple_assign_lhs (stmt), nontrapping, true);
+	  add_or_mark_expr (bb, gimple_assign_rhs1 (stmt), nontrapping, false);
 	}
     }
 }
Index: tree-ssa-uncprop.c
===================================================================
--- tree-ssa-uncprop.c	(revision 202698)
+++ tree-ssa-uncprop.c	(working copy)
@@ -360,11 +360,11 @@ public:
   uncprop_dom_walker (cdi_direction direction)
     : dom_walker (direction)
   {
-    equiv_stack_.create (2);
+    equiv_stack.create (2);
   }
   ~uncprop_dom_walker ()
   {
-    equiv_stack_.release ();
+    equiv_stack.release ();
   }
 
   virtual void before_dom_children (basic_block);
@@ -376,7 +376,7 @@ private:
    leading to this block.  If no such edge equivalency exists, then we
    record NULL.  These equivalences are live until we leave the dominator
    subtree rooted at the block where we record the equivalency.  */
-  vec<tree> equiv_stack_;
+  vec<tree> equiv_stack;
 };
 
 /* Main driver for un-cprop.  */
@@ -428,7 +428,7 @@ void
 uncprop_dom_walker::after_dom_children (basic_block bb ATTRIBUTE_UNUSED)
 {
   /* Pop the topmost value off the equiv stack.  */
-  tree value = equiv_stack_.pop ();
+  tree value = equiv_stack.pop ();
 
   /* If that value was non-null, then pop the topmost equivalency off
      its equivalency stack.  */
@@ -566,13 +566,13 @@ uncprop_dom_walker::before_dom_children
 	  struct edge_equivalency *equiv = (struct edge_equivalency *) e->aux;
 
 	  record_equiv (equiv->rhs, equiv->lhs);
-	  equiv_stack_.safe_push (equiv->rhs);
+	  equiv_stack.safe_push (equiv->rhs);
 	  recorded = true;
 	}
     }
 
   if (!recorded)
-    equiv_stack_.safe_push (NULL_TREE);
+    equiv_stack.safe_push (NULL_TREE);
 
   uncprop_into_successor_phis (bb);
 }
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 202698)
+++ ChangeLog	(working copy)
@@ -91,28 +91,29 @@
 	(move_computations_stmt): Convert to method
 	move_computations_dom_walker::before_dom_children.
 	(move_computations, tree_ssa_lim): Adjust.
-	* tree-ssa-phiopt.c (nontrapping_dom_walker): new class
-	(nt_init_block): Make method
+	* tree-ssa-phiopt.c (nontrapping_dom_walker): New class.
+	(nt_init_block): Convert to method
 	notrappping_dom_walker::before_dom_children.
-	(nt_fini_block): Make
+	(nt_fini_block): Convert to method
 	method nontrapping_dom_walker::after_dom_children.
 	(get_non_trapping): Adjust.
 	* tree-ssa-pre.c (eliminate_dom_walker): New class.
-	(eliminate_bb): Make method eliminate_dom_walker::before_dom_children.
-	(eliminate_leave_block): Make method.
+	(eliminate_bb): Convert to method
+	eliminate_dom_walker::before_dom_children.
+	(eliminate_leave_block): Convert to method
 	eliminate_dom_walker::after_dom_children.
-	(eliminate): Adjust
+	(eliminate): Adjust.
 	* tree-ssa-strlen.c (strlen_dom_walker): New class.
-	(strlen_enter_block): Make method
+	(strlen_enter_block): Convert to method
 	strlen_dom_walker::before_dom_children.
-	(strlen_leave_block): Make
+	(strlen_leave_block): Convert to method
 	method strlen_dom_walker::after_dom_children.
 	(tree_ssa_strlen): Adjust.
 	* tree-ssa-uncprop.c (uncprop_dom_walker): New class.
 	(tree_ssa_uncprop): Adjust.
-	(uncprop_leave_block): Make method
+	(uncprop_leave_block): Convert to method
 	uncprop_dom_walker::after_dom_children.
-	(uncprop_leave_block): Make method
+	(uncprop_leave_block): Convert to method
 	uncprop_dom_walker::before_dom_children.
 
 2013-09-18  Bin Cheng  <bin.cheng@arm.com>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]