[gomp4.1] comment some stuff
diff mbox

Message ID 55DCA769.3070305@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Aug. 25, 2015, 5:35 p.m. UTC
I'm obviously not smart enough to understand libgomp's tasking runtime, 
and rth and you get 0 for commenting skills ;-).

I had some notes scribbled down while reading the code, and figured 
someone else might read this code some day.  It's still in dire need of 
commenting, but this mildly helps.

OK for branch?
commit 5fc2816946c9250c4cca43d002b364b2d6400919
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Tue Aug 25 10:32:48 2015 -0700

    	* env.c: Make gomp_max_task_priority_var static.
    	* libgomp.h (struct gomp_task_depend_entry): Add comment.
    	* task.c (gomp_clear_parent): Document function.
    	(GOMP_task): Same.
    	(gomp_task_run_pre): Add comments.
    	(gomp_task_run_post_handle_dependers): Same.
    	(gomp_task_run_post_remove_parent): Same.
    	(gomp_task_run_post_remove_taskgroup): Same.
    	(GOMP_taskwait): Same.
    	(gomp_task_maybe_wait_for_dependencies): Same.

Comments

Aldy Hernandez Aug. 25, 2015, 5:36 p.m. UTC | #1
On 08/25/2015 10:35 AM, Aldy Hernandez wrote:
> -int gomp_max_task_priority_var = 0;
> +static int gomp_max_task_priority_var = 0;

Sorry I snuck that in there.  The variable is unused elsewhere, might as 
well make it static.

Aldy
Jakub Jelinek Aug. 26, 2015, 1:27 p.m. UTC | #2
On Tue, Aug 25, 2015 at 10:35:37AM -0700, Aldy Hernandez wrote:
> diff --git a/libgomp/env.c b/libgomp/env.c
> index 65a6851..0569521 100644
> --- a/libgomp/env.c
> +++ b/libgomp/env.c
> @@ -69,7 +69,7 @@ struct gomp_task_icv gomp_global_icv = {
>  
>  unsigned long gomp_max_active_levels_var = INT_MAX;
>  bool gomp_cancel_var = false;
> -int gomp_max_task_priority_var = 0;
> +static int gomp_max_task_priority_var = 0;
>  #ifndef HAVE_SYNC_BUILTINS
>  gomp_mutex_t gomp_managed_threads_lock;
>  #endif

Please remove this hunk.  The variable is meant to be used in task.c,
where
  (void) priority;
is present right now (like:
  if (priority > gomp_max_task_priority_var)
    priority = gomp_max_task_priority_var;
or so.

> @@ -110,7 +112,12 @@ static void gomp_task_maybe_wait_for_dependencies (void **depend);
>  
>  /* Called when encountering an explicit task directive.  If IF_CLAUSE is
>     false, then we must not delay in executing the task.  If UNTIED is true,
> -   then the task may be executed by any member of the team.  */
> +   then the task may be executed by any member of the team.
> +
> +   DEPEND is an array containing:
> +	depend[0]: number of depend elements.
> +	depend[1]: number of depend elements of type "out".
> +	depend[N+2]: address of [0..N]th depend element.  */

Either [1..N]th, or [0..N-1]th.  And depend[N+2] should better be
depend[2..N+1].

Otherwise LGTM.

	Jakub

Patch
diff mbox

diff --git a/libgomp/env.c b/libgomp/env.c
index 65a6851..0569521 100644
--- a/libgomp/env.c
+++ b/libgomp/env.c
@@ -69,7 +69,7 @@  struct gomp_task_icv gomp_global_icv = {
 
 unsigned long gomp_max_active_levels_var = INT_MAX;
 bool gomp_cancel_var = false;
-int gomp_max_task_priority_var = 0;
+static int gomp_max_task_priority_var = 0;
 #ifndef HAVE_SYNC_BUILTINS
 gomp_mutex_t gomp_managed_threads_lock;
 #endif
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 9031649..3d705ef 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -279,6 +279,7 @@  struct gomp_task_depend_entry
   struct gomp_task_depend_entry *next;
   struct gomp_task_depend_entry *prev;
   struct gomp_task *task;
+  /* Depend entry is of type "IN".  */
   bool is_in;
   bool redundant;
   bool redundant_out;
diff --git a/libgomp/task.c b/libgomp/task.c
index f2a0fae..7c7bae4 100644
--- a/libgomp/task.c
+++ b/libgomp/task.c
@@ -92,6 +92,8 @@  gomp_end_task (void)
   thr->task = task->parent;
 }
 
+/* Orphan the task in CHILDREN and all its siblings.  */
+
 static inline void
 gomp_clear_parent (struct gomp_task *children)
 {
@@ -110,7 +112,12 @@  static void gomp_task_maybe_wait_for_dependencies (void **depend);
 
 /* Called when encountering an explicit task directive.  If IF_CLAUSE is
    false, then we must not delay in executing the task.  If UNTIED is true,
-   then the task may be executed by any member of the team.  */
+   then the task may be executed by any member of the team.
+
+   DEPEND is an array containing:
+	depend[0]: number of depend elements.
+	depend[1]: number of depend elements of type "out".
+	depend[N+2]: address of [0..N]th depend element.  */
 
 void
 GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
@@ -444,8 +451,10 @@  gomp_task_run_pre (struct gomp_task *child_task, struct gomp_task *parent,
 {
   if (parent)
     {
+      /* Remove child_task from parent.  */
       if (parent->children == child_task)
 	parent->children = child_task->next_child;
+
       if (__builtin_expect (child_task->parent_depends_on, 0)
 	  && parent->taskwait->last_parent_depends_on == child_task)
 	{
@@ -456,8 +465,10 @@  gomp_task_run_pre (struct gomp_task *child_task, struct gomp_task *parent,
 	    parent->taskwait->last_parent_depends_on = NULL;
 	}
     }
+  /* Remove child_task from taskgroup.  */
   if (taskgroup && taskgroup->children == child_task)
     taskgroup->children = child_task->next_taskgroup;
+
   child_task->prev_queue->next_queue = child_task->next_queue;
   child_task->next_queue->prev_queue = child_task->prev_queue;
   if (team->task_queue == child_task)
@@ -528,6 +539,7 @@  gomp_task_run_post_handle_dependers (struct gomp_task *child_task,
 	      if (parent->taskwait && parent->taskwait->last_parent_depends_on
 		  && !task->parent_depends_on)
 		{
+		  /* Put task in last_parent_depends_on.  */
 		  struct gomp_task *last_parent_depends_on
 		    = parent->taskwait->last_parent_depends_on;
 		  task->next_child = last_parent_depends_on->next_child;
@@ -535,6 +547,7 @@  gomp_task_run_post_handle_dependers (struct gomp_task *child_task,
 		}
 	      else
 		{
+		  /* Put task at the top of the sibling list.  */
 		  task->next_child = parent->children;
 		  task->prev_child = parent->children->prev_child;
 		  parent->children = task;
@@ -544,6 +557,7 @@  gomp_task_run_post_handle_dependers (struct gomp_task *child_task,
 	    }
 	  else
 	    {
+	      /* Put task in the sibling list.  */
 	      task->next_child = task;
 	      task->prev_child = task;
 	      parent->children = task;
@@ -628,12 +642,18 @@  gomp_task_run_post_handle_depend (struct gomp_task *child_task,
   return gomp_task_run_post_handle_dependers (child_task, team);
 }
 
+/* Remove CHILD_TASK from its parent.  */
+
 static inline void
 gomp_task_run_post_remove_parent (struct gomp_task *child_task)
 {
   struct gomp_task *parent = child_task->parent;
   if (parent == NULL)
     return;
+
+  /* If this was the last task the parent was depending on,
+     synchronize with gomp_task_maybe_wait_for_dependencies so it can
+     clean up and return.  */
   if (__builtin_expect (child_task->parent_depends_on, 0)
       && --parent->taskwait->n_depend == 0
       && parent->taskwait->in_depend_wait)
@@ -641,6 +661,8 @@  gomp_task_run_post_remove_parent (struct gomp_task *child_task)
       parent->taskwait->in_depend_wait = false;
       gomp_sem_post (&parent->taskwait->taskwait_sem);
     }
+
+  /* Remove CHILD_TASK from its sibling list.  */
   child_task->prev_child->next_child = child_task->next_child;
   child_task->next_child->prev_child = child_task->prev_child;
   if (parent->children != child_task)
@@ -663,6 +685,8 @@  gomp_task_run_post_remove_parent (struct gomp_task *child_task)
     }
 }
 
+/* Remove CHILD_TASK from its taskgroup.  */
+
 static inline void
 gomp_task_run_post_remove_taskgroup (struct gomp_task *child_task)
 {
@@ -889,6 +913,9 @@  GOMP_taskwait (void)
 	 finish_cancelled:;
 	  size_t new_tasks
 	    = gomp_task_run_post_handle_depend (child_task, team);
+
+	  /* Remove child_task from children list, and set up the next
+	     sibling to be run.  */
 	  child_task->prev_child->next_child = child_task->next_child;
 	  child_task->next_child->prev_child = child_task->prev_child;
 	  if (task->children == child_task)
@@ -898,8 +925,12 @@  GOMP_taskwait (void)
 	      else
 		task->children = NULL;
 	    }
+	  /* Orphan all the children of CHILD_TASK.  */
 	  gomp_clear_parent (child_task->children);
+
+	  /* Remove CHILD_TASK from its taskgroup.  */
 	  gomp_task_run_post_remove_taskgroup (child_task);
+
 	  to_free = child_task;
 	  child_task = NULL;
 	  team->task_count--;
@@ -915,7 +946,9 @@  GOMP_taskwait (void)
 }
 
 /* This is like GOMP_taskwait, but we only wait for tasks that the
-   upcoming task depends on.  */
+   upcoming task depends on.
+
+   DEPEND is as in GOMP_task.  */
 
 static void
 gomp_task_maybe_wait_for_dependencies (void **depend)
@@ -956,8 +989,10 @@  gomp_task_maybe_wait_for_dependencies (void **depend)
 		       to front, so that we run it as soon as possible.  */
 		    if (last_parent_depends_on)
 		      {
+			/* Remove tsk from the sibling list...  */
 			tsk->prev_child->next_child = tsk->next_child;
 			tsk->next_child->prev_child = tsk->prev_child;
+			/* ...and insert it into last_parent_depends_on.  */
 			tsk->prev_child = last_parent_depends_on;
 			tsk->next_child = last_parent_depends_on->next_child;
 			tsk->prev_child->next_child = tsk;
@@ -965,8 +1000,11 @@  gomp_task_maybe_wait_for_dependencies (void **depend)
 		      }
 		    else if (tsk != task->children)
 		      {
+			/* Remove tsk from the sibling list...  */
 			tsk->prev_child->next_child = tsk->next_child;
 			tsk->next_child->prev_child = tsk->prev_child;
+			/* ...and insert it into the running task's
+			   children.  */
 			tsk->prev_child = task->children;
 			tsk->next_child = task->children->next_child;
 			task->children = tsk;
@@ -1054,6 +1092,8 @@  gomp_task_maybe_wait_for_dependencies (void **depend)
 	    = gomp_task_run_post_handle_depend (child_task, team);
 	  if (child_task->parent_depends_on)
 	    --taskwait.n_depend;
+
+	  /* Remove child_task from sibling list.  */
 	  child_task->prev_child->next_child = child_task->next_child;
 	  child_task->next_child->prev_child = child_task->prev_child;
 	  if (task->children == child_task)
@@ -1063,6 +1103,7 @@  gomp_task_maybe_wait_for_dependencies (void **depend)
 	      else
 		task->children = NULL;
 	    }
+
 	  gomp_clear_parent (child_task->children);
 	  gomp_task_run_post_remove_taskgroup (child_task);
 	  to_free = child_task;