[Starpu-devel] [Review needed] nreaders might be 0 in _starpu_add_writer_after_readers.

[Cyril Roelandt <cyril.roelandt@inria.fr>]

Hi,

In src/core/dependencies/implicit_data_deps.c lies the following code :

unsigned nreaders = 0;
...
while (l)
{
        if (l->task != post_sync_task)
                nreaders++;
        l = l->next;
}

...

struct starpu_task *task_array[nreaders];


We probably do not want to write this last line if nreaders is 0, since 
variable length arrays must have a size greater than 0. So we could just 
write this instead :

struct starpu_task *task_array[nreaders+1];

That would be a little hacky, but still acceptable. The thing is, later, 
we call starpu_task_declare_deps_array :

starpu_task_declare_deps_array(pre_sync_task, nreaders, task_array);

which immediatly returns if nreaders is equal to 0.


The following patch makes sure that task_array is only declared if 
nreaders is greater than 0 (thus removing the need to allocate an array 
of size nreaders+1) and that starpu_task_declare_deps_array is only 
called if nreaders is greater than 0 (which may be a good thing for the 
execution speed).

Index: src/core/dependencies/implicit_data_deps.c
===================================================================
--- src/core/dependencies/implicit_data_deps.c  (revision 5186)
+++ src/core/dependencies/implicit_data_deps.c  (working copy)
@@ -80,21 +80,25 @@
        }
        _STARPU_DEP_DEBUG("%d readers\n", nreaders);

-       /* Put all tasks in the list into task_array */
-       struct starpu_task *task_array[nreaders];
-       unsigned i = 0;
-       l = handle->last_submitted_readers;
-       while (l)
+       if (nreaders > 0)
        {
-               STARPU_ASSERT(l->task);
-               if (l->task != post_sync_task) {
-                       task_array[i++] = l->task;
-                       _STARPU_DEP_DEBUG("dep %p -> %p\n", l->task, 
pre_sync_task);
+               /* Put all tasks in the list into task_array */
+               struct starpu_task *task_array[nreaders];
+               unsigned i = 0;
+               l = handle->last_submitted_readers;
+               while (l)
+               {
+                       STARPU_ASSERT(l->task);
+                       if (l->task != post_sync_task) {
+                               task_array[i++] = l->task;
+                               _STARPU_DEP_DEBUG("dep %p -> %p\n", 
l->task, pre_sync_task);
+                       }
+
+                       struct _starpu_task_wrapper_list *prev = l;
+                       l = l->next;
+                       free(prev);
                }
-
-               struct _starpu_task_wrapper_list *prev = l;
-               l = l->next;
-               free(prev);
+               starpu_task_declare_deps_array(pre_sync_task, nreaders, 
task_array);
        }
 #ifndef STARPU_USE_FXT
        if (_starpu_bound_recording)
@@ -121,7 +125,6 @@
        handle->last_submitted_readers = NULL;
        handle->last_submitted_writer = post_sync_task;

-       starpu_task_declare_deps_array(pre_sync_task, nreaders, task_array);
 }
 /* Write after Write (WAW) */
 static void _starpu_add_writer_after_writer(starpu_data_handle_t 
handle, struct starpu_task *pre_sync_task, struct starpu_task 
*post_sync_task)


make check is happy with that, but I am not sure it does the exact same 
thing as the original code. My main concern is about pre_sync_task, 
which will be used by starpu_task_declare_deps_array() before being used 
when declaring dependencies with ghost readers. Is that a problem ? Is 
the code still as easy to understand as before ?


Cyril.