GNU bug report logs - #6728
[PATCH] sort: omit 'restrict' in doubtful cases

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 6728 in the body.
You can then email your comments to 6728 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> CS.UCLA.EDU>
To: Bug Coreutils <bug-coreutils <at> gnu.org>
Subject: [PATCH] sort: omit 'restrict' in doubtful cases
Date: Sun, 25 Jul 2010 20:29:45 -0700

Several of the uses of 'restrict' in sort.c are not needed,
and the one in the typedef might even be incorrect (though
it would be hard to come up with a test case proving this,
as it is compiler-dependent and would require a genius compiler).
I installed the following.  It does not affect the code generated
on RHEL 5 x86-64 (gcc -O2), so any performance penalty of omitting
these 'restrict's is likely to be minor.

From f9f86499bf0d45df5e304ce014724d35f3386560 Mon Sep 17 00:00:00 2001
From: Paul R. Eggert <eggert <at> cs.ucla.edu>
Date: Sun, 25 Jul 2010 20:25:31 -0700
Subject: [PATCH] sort: omit 'restrict' in doubtful cases

* src/sort.c (lock_node, unlock_node, queue_destroy, queue_init):
(queue_pop):
Omit 'restrict'; it shouldn't help here, as these functions have just
one pointer parameter and don't access static storage.
(queue_insert, check_insert, update_parent): Omit 'restrict', as
the pointer types differ, and are not char * or unsigned char *,
and therefore can't alias.
(write_unique): Omit 'restrict', as the pointer types are all
read-only.
(merge_loop, sortlines): Omit 'restrict', as any performance
advantages are extremely unlikely and it's not worth cluttering
the code for that.
(struct thread_args): Omit 'restrict': this seems to be incorrect.
It's unlikely for 'restrict' to be correct inside a typedef.
---
 src/sort.c |   36 +++++++++++++++++-------------------
 1 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index ca1d95c..ea2720f 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -3119,7 +3119,7 @@ compare_nodes (void const *a, void const *b)
    number of processors.  */
 
 static inline void
-lock_node (struct merge_node *restrict node)
+lock_node (struct merge_node *node)
 {
   pthread_spin_lock (node->lock);
 }
@@ -3127,7 +3127,7 @@ lock_node (struct merge_node *restrict node)
 /* Unlock a merge tree NODE. */
 
 static inline void
-unlock_node (struct merge_node *restrict node)
+unlock_node (struct merge_node *node)
 {
   pthread_spin_unlock (node->lock);
 }
@@ -3135,7 +3135,7 @@ unlock_node (struct merge_node *restrict node)
 /* Destroy merge QUEUE. */
 
 static inline void
-queue_destroy (struct merge_node_queue *restrict queue)
+queue_destroy (struct merge_node_queue *queue)
 {
   heap_free (queue->priority_queue);
   pthread_cond_destroy (&queue->cond);
@@ -3148,7 +3148,7 @@ queue_destroy (struct merge_node_queue *restrict queue)
    heap, RESERVE should be 2 * NTHREADS. */
 
 static inline void
-queue_init (struct merge_node_queue *restrict queue, size_t reserve)
+queue_init (struct merge_node_queue *queue, size_t reserve)
 {
   queue->priority_queue = heap_alloc (compare_nodes, reserve);
   pthread_mutex_init (&queue->mutex, NULL);
@@ -3159,8 +3159,7 @@ queue_init (struct merge_node_queue *restrict queue, size_t reserve)
    or does not need to lock NODE. */
 
 static inline void
-queue_insert (struct merge_node_queue *restrict queue,
-              struct merge_node *restrict node)
+queue_insert (struct merge_node_queue *queue, struct merge_node *node)
 {
   pthread_mutex_lock (&queue->mutex);
   heap_insert (queue->priority_queue, node);
@@ -3172,7 +3171,7 @@ queue_insert (struct merge_node_queue *restrict queue,
 /* Pop NODE off priority QUEUE. Guarantee a non-null, spinlocked NODE. */
 
 static inline struct merge_node *
-queue_pop (struct merge_node_queue *restrict queue)
+queue_pop (struct merge_node_queue *queue)
 {
   struct merge_node *node = NULL;
 
@@ -3199,8 +3198,7 @@ queue_pop (struct merge_node_queue *restrict queue)
    thus is only appropriate for internal sort. */
 
 static inline void
-write_unique (struct line const *restrict line, FILE *tfp,
-              char const *temp_output)
+write_unique (struct line const *line, FILE *tfp, char const *temp_output)
 {
   static struct line const *saved = NULL;
 
@@ -3284,7 +3282,7 @@ mergelines_node (struct merge_node *restrict node, size_t total_lines,
 /* Insert NODE into QUEUE if it passes insertion checks. */
 
 static inline void
-check_insert (struct merge_node *node, struct merge_node_queue *restrict queue)
+check_insert (struct merge_node *node, struct merge_node_queue *queue)
 {
   size_t lo_avail = node->lo - node->end_lo;
   size_t hi_avail = node->hi - node->end_hi;
@@ -3304,8 +3302,8 @@ check_insert (struct merge_node *node, struct merge_node_queue *restrict queue)
 /* Update parent merge tree NODE. */
 
 static inline void
-update_parent (struct merge_node *restrict node, size_t merged,
-               struct merge_node_queue *restrict queue)
+update_parent (struct merge_node *node, size_t merged,
+               struct merge_node_queue *queue)
 {
   if (node->level > MERGE_ROOT)
     {
@@ -3326,7 +3324,7 @@ update_parent (struct merge_node *restrict node, size_t merged,
    some of those lines, until the MERGE_END node is popped. */
 
 static void
-merge_loop (struct merge_node_queue *restrict queue,
+merge_loop (struct merge_node_queue *queue,
             size_t total_lines, FILE *tfp, char const *temp_output)
 {
   while (1)
@@ -3352,8 +3350,8 @@ merge_loop (struct merge_node_queue *restrict queue,
 
 static void sortlines (struct line *restrict, struct line *restrict,
                        unsigned long int, size_t,
-                       struct merge_node *restrict, bool,
-                       struct merge_node_queue *restrict,
+                       struct merge_node *, bool,
+                       struct merge_node_queue *,
                        FILE *, char const *);
 
 /* Thread arguments for sortlines_thread. */
@@ -3364,9 +3362,9 @@ struct thread_args
   struct line *dest;
   unsigned long int nthreads;
   size_t const total_lines;
-  struct merge_node *const restrict parent;
+  struct merge_node *const parent;
   bool lo_child;
-  struct merge_node_queue *const restrict merge_queue;
+  struct merge_node_queue *const merge_queue;
   FILE *tfp;
   char const *output_temp;
 };
@@ -3407,8 +3405,8 @@ sortlines_thread (void *data)
 static void
 sortlines (struct line *restrict lines, struct line *restrict dest,
            unsigned long int nthreads, size_t total_lines,
-           struct merge_node *restrict parent, bool lo_child,
-           struct merge_node_queue *restrict merge_queue,
+           struct merge_node *parent, bool lo_child,
+           struct merge_node_queue *merge_queue,
            FILE *tfp, char const *temp_output)
 {
   /* Create merge tree NODE. */
-- 
1.7.1

Message #10 received at 6728-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> CS.UCLA.EDU>
Cc: 6728-done <at> debbugs.gnu.org
Subject: Re: bug#6728: [PATCH] sort: omit 'restrict' in doubtful cases
Date: Mon, 26 Jul 2010 10:47:06 +0100

On 26/07/10 04:29, Paul Eggert wrote:
> Several of the uses of 'restrict' in sort.c are not needed,

Right. Would only be useful for multiple pointers of the same type.
Closing...

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.