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: [Ping] [patch 0/3] New macro PREFERRED_RENAME_CLASS


> Patch 1: does everything except adding target hook
> preferred_rename_class .  It is a separate improvement to register-rename.
> http://gcc.gnu.org/ml/gcc-patches/2010-10/msg02197.html

You didn't really inline the callback, did you?  If you want to keep the 
comparer function standalone, then remove the superfluous indirection.


@@ -51,6 +51,8 @@ struct du_head
   struct du_head *next_chain;
   /* The first and last elements of this chain.  */
   struct du_chain *first, *last;
+  /* The length of du_chain for non-debug insns.  */
+  int length;
   /* Describes the register being tracked.  */
   unsigned regno, nregs;
 
du_chain is a structure tag, so you need to elaborate.



@@ -154,6 +156,141 @@ merge_overlapping_regs (HARD_REG_SET *pset, struct 
du_head *head)
     }
 }
 
+/* Get the next LENGTH element in list from START.  If length of
+   list from START is less than or equal to LENGTH, return the 
+   last element.  */
+static struct du_head *
+get_tail_du_head (struct du_head *start, int length)
+{
+  while (length-- && start->next_chain != NULL)
+      start = start->next_chain;
+
+  return start;

Avoid TAIL and HEAD in the same function name.  GET_ELEMENT is clearer.  And 
LENGTH is a poor choice for the parameter name, use N instead.

/* Return the Nth element in LIST.  If LIST contains less than N elements,
   return the last one.  */

static struct du_head *
get_element (struct du_head *list, int n)



+/* Merge the first two sequences of CURRENT_LENGTH nodes in the 
+   linked list pointed by START_NODE.  Update START_NODE to point 
+   to the merged nodes, and return a pointer to the last merged
+   node.   CMP is callback  for comparison.  */
+
+static struct du_head *
+merge(struct du_head **start_node, int current_length,
+      int (*cmp) (const struct du_head *, const struct du_head *))

Why CURRENT_LENGTH?  Just use LENGTH.

The first sentence is somewhat ambiguous as one might get the impression that 
the 2 sequences aren't already present in the list.

/* Merge the first 2 sub-lists of LENGTH nodes contained in the linked list
   pointed to by START_NODE.  Update...

Document what happens when the linked list doesn't contain enough elements.


+	  current_tail_node = 
+	    get_tail_du_head (current_sorted_node,
+			      current_length - left_count);

'=' on the next line


+  return current_tail_node->next_chain != NULL ? 
+    current_tail_node : NULL;

return (current_tail_node->next_chain ? current_tail_node : NULL);


+/* Non-recursive merge sort to du_head list.  */
+static void
+sort_du_head (struct du_head **head,
+	      int(*cmp)(const struct du_head *, const struct du_head *))

/* Sort the linked list pointed to by HEAD.  The algorithm is a non-recursive
   merge sort [that ...optional more detailed explanation...]


+  do
+    {
+      struct du_head **segment = head;
+      

Trailing spaces.

+      while ((last_tail = merge(segment, current_length, cmp)) 
+	     != NULL)

Why breaking the line?

+	{
+	  segment = &last_tail->next_chain;
+	}

Superfluous curly braces.


Are you sure that this revised implementation works as expected?  Won't it 
stop after the 1-length merge phase?


@@ -265,13 +405,13 @@ regrename_optimize (void)
 
 	  /* Now potential_regs is a reasonable approximation, let's
 	     have a closer look at each register still in there.  */
-	  for (new_reg = 0; new_reg < FIRST_PSEUDO_REGISTER; new_reg++)
+	  for (new_reg = FIRST_PSEUDO_REGISTER - 1; new_reg >= 0; new_reg--)

What is this hunk for?  It isn't mentioned in the ChangeLog.

-- 
Eric Botcazou


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