This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Ping] [patch 0/3] New macro PREFERRED_RENAME_CLASS
- From: Eric Botcazou <ebotcazou at adacore dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Fri, 5 Nov 2010 09:55:56 +0100
- Subject: Re: [Ping] [patch 0/3] New macro PREFERRED_RENAME_CLASS
- References: <4CD24091.1020904@codesourcery.com>
> 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