Re: [Patch, Fortra, Coarray] Free allocated memory for static coarrays.

[Daniel Carrera <dcarrera@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

Hi Tobias,


New patch attached. In addition to the fixes you listed I also had to 
fix the call to MPI_Allgather. More below:


> Between "size," and "caf_register_t" is a<tab>  instead of a simple " ".

Fixed.

> As you mentioned yourself (in a private mail, commenting on my code):
> There is a "*" missing at the left side of the assignment lines.

Oops. Fixed.

> Wrong order for the "token" allocation: You first need to know how many
> images exists, before you can use "caf_num_images".

OOps again. Fixed.


> +  MPI_Allgather (local,    sizeof (void*), MPI_BYTE,
>
> There is a<tab>  character between "local," and "sizeof"; it should
> have been a space.

I was trying to make the code look nicer by making things lineup. It 
also helps me find typos. In general, is it ok to add spaces to make 
code lineup?



> Have you tested the modifications with MPI? I mean: Compiling mpi.c,
> and then in gcc/testsuite/gfortran.dg/coarray:

Ooops. I compiled and ran the test suite but I forgot to test MPI 
separately. There was a bug in the MPI_Allgather line. It turns out that 
my earlier "&local" was correct after all:

   MPI_Allgather (&local, sizeof (void*), MPI_BYTE,
		 token,  sizeof (void*), MPI_BYTE, MPI_COMM_WORLD);



The first parameter of MPI_Allgather is not the data to send, but a 
pointer to the data you want to send. That makes sense, otherwise you 
wouldn't be able to send large data. Anyway, since what we want to send 
is "local" (i.e. the address of the coarray) we need to pass a pointer 
to local, i.e. &local.

Anyway, now the MPI tests all run correctly. I also recompiled and 
re-ran the test suite with "make check-gfortran" and those are fine too.

As before, I can't make an automated test for memory allocation, but I 
used fprintf statements to confirm that "token[caf_this_image-1]" has 
the same value as "local" for the rest "registering_1.f90". Of course I 
removed those for the final patch, but at least I feel confidence that 
the patch does what it should.

Cheers,
Daniel.
-- 
I'm not overweight, I'm undertall.

[-- Attachment #2: caf-free-memory.patch --]
[-- Type: text/x-patch, Size: 4744 bytes --]

Index: libgfortran/caf/single.c
===================================================================
--- libgfortran/caf/single.c	(revision 174944)
+++ libgfortran/caf/single.c	(working copy)
@@ -35,7 +35,10 @@
    Note: For performance reasons -fcoarry=single should be used
    rather than this library.  */
 
+/* Global variables.  */
+caf_static_t *caf_static_list = NULL;
 
+
 void
 _gfortran_caf_init (int *argc __attribute__ ((unused)),
 		    char ***argv __attribute__ ((unused)),
@@ -49,16 +52,31 @@
 void
 _gfortran_caf_finalize (void)
 {
+  while (caf_static_list != NULL)
+    {
+      free(caf_static_list->token[0]);
+      caf_static_list = caf_static_list->prev;
+    }
 }
 
 
 void *
-_gfortran_caf_register (ptrdiff_t size,
-			caf_register_t type __attribute__ ((unused)),
+_gfortran_caf_register (ptrdiff_t size, caf_register_t type,
 			void **token)
 {
-  *token = NULL;
-  return malloc (size);
+  void *local;
+  
+  local = malloc (size);
+  token = malloc (sizeof (void*) * 1);
+
+  if (type == CAF_REGTYPE_COARRAY_STATIC)
+    {
+      caf_static_t *tmp = malloc (sizeof (caf_static_t));
+      tmp->prev  = caf_static_list;
+      tmp->token = token;
+      caf_static_list = tmp;
+    }
+  return local;
 }
 
 
Index: libgfortran/caf/libcaf.h
===================================================================
--- libgfortran/caf/libcaf.h	(revision 174944)
+++ libgfortran/caf/libcaf.h	(working copy)
@@ -38,15 +38,23 @@
 #define STAT_LOCKED_OTHER_IMAGE	2
 #define STAT_STOPPED_IMAGE 	3
 
-
+/* Describes what type of array we are registerring.  */
 typedef enum caf_register_t {
-  CAF_REGTYPE_COARRAY,
+  CAF_REGTYPE_COARRAY_STATIC,
+  CAF_REGTYPE_COARRAY_ALLOC,
   CAF_REGTYPE_LOCK,
   CAF_REGTYPE_LOCK_COMP 
 }
 caf_register_t;
 
+/* Linked list of static coarrays registered.  */
+typedef struct caf_static_t {
+  void **token;
+  struct caf_static_t *prev;
+}
+caf_static_t;
 
+
 void _gfortran_caf_init (int *, char ***, int *, int *);
 void _gfortran_caf_finalize (void);
 
Index: libgfortran/caf/mpi.c
===================================================================
--- libgfortran/caf/mpi.c	(revision 174944)
+++ libgfortran/caf/mpi.c	(working copy)
@@ -42,7 +42,10 @@
 static int caf_this_image;
 static int caf_num_images;
 
+caf_static_t *caf_static_list = NULL;
 
+
+
 /* Initialize coarray program.  This routine assumes that no other
    MPI initialization happened before; otherwise MPI_Initialized
    had to be used.  As the MPI library might modify the command-line
@@ -52,36 +55,69 @@
 void
 _gfortran_caf_init (int *argc, char ***argv, int *this_image, int *num_images)
 {
-  /* caf_mpi_initialized is only true if the main program is not written in
-     Fortran.  */
-  MPI_Initialized (&caf_mpi_initialized);
-  if (!caf_mpi_initialized)
-    MPI_Init (argc, argv);
+  if (caf_num_images == 0)
+    {
+      /* caf_mpi_initialized is only true if the main program is
+       not written in Fortran.  */
+      MPI_Initialized (&caf_mpi_initialized);
+      if (!caf_mpi_initialized)
+	MPI_Init (argc, argv);
 
-  MPI_Comm_rank (MPI_COMM_WORLD, &caf_this_image);
-  *this_image = ++caf_this_image;
-  MPI_Comm_size (MPI_COMM_WORLD, &caf_num_images);
-  *num_images = caf_num_images;
+      MPI_Comm_size (MPI_COMM_WORLD, &caf_num_images);
+      MPI_Comm_rank (MPI_COMM_WORLD, &caf_this_image);
+      caf_this_image++;
+    }
+
+  if (this_image)
+    *this_image = caf_this_image;
+  if (num_images)
+    *num_images = caf_num_images; 
 }
 
 
+
 /* Finalize coarray program.   */
 
 void
 _gfortran_caf_finalize (void)
 {
+  while (caf_static_list != NULL)
+    {
+      free(caf_static_list->token[caf_this_image-1]);
+      caf_static_list = caf_static_list->prev;
+    }
+
   if (!caf_mpi_initialized)
     MPI_Finalize ();
 }
 
 
 void *
-_gfortran_caf_register (ptrdiff_t size,
-                        caf_register_t type __attribute__ ((unused)),
+_gfortran_caf_register (ptrdiff_t size, caf_register_t type,
                         void **token)
 {
-  *token = NULL;
-  return malloc (size);
+  void *local;
+
+  /* Start MPI if not already started.  */
+  if (caf_num_images == 0)
+    _gfortran_caf_init (NULL, NULL, NULL, NULL);
+
+  /* Token contains only a list of pointers.  */
+  local = malloc (size);
+  token = malloc (sizeof (void*) * caf_num_images);
+
+  /* token[img-1] is the address of the token in image "img".  */
+  MPI_Allgather (&local, sizeof (void*), MPI_BYTE,
+		 token,  sizeof (void*), MPI_BYTE, MPI_COMM_WORLD);
+
+  if (type == CAF_REGTYPE_COARRAY_STATIC)
+    {
+      caf_static_t *tmp = malloc (sizeof (caf_static_t));
+      tmp->prev  = caf_static_list;
+      tmp->token = token;
+      caf_static_list = tmp;
+    }
+  return local;
 }