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

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

[Tobias Burnus]

Hi Daniel,

Daniel Carrera wrote:
> This patch adds a linked list to keep track of the allocated
> memory of all static coarrays and ensures that at the end of
> the program the memory is freed.

+_gfortran_caf_register (ptrdiff_t size,	caf_register_t type,
 			void **token)

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


 _gfortran_caf_init (int *argc, char ***argv, int *this_image, int *num_images)
[...]
+  if (this_image)
+    this_image = caf_this_image;
+  if (num_images)
+    num_images = caf_num_images;

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


+_gfortran_caf_register (ptrdiff_t size, caf_register_t type,
                         void **token)
[...]
+  token = malloc (sizeof (void*) * caf_num_images);
+
+  /* Start MPI if not already started.  */
+  if (caf_num_images == 0)
+    _gfortran_caf_init (NULL, NULL, NULL, NULL);

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


+  MPI_Allgather (local,    sizeof (void*), MPI_BYTE,

There is a <tab> character between "local," and "sizeof"; it should
have been a space.


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

for I in *.f90; do echo ====== $I =====; \
  mpif90 -fcoarray=lib $I mpi.o && mpiexec -np 3 ./a.out; done

Additionally, have you run test suite? It is sufficient to run recompile
gfortran (to create the latest libcaf_single.a from single.c) and then to
run
  make check-gfortran RUNTESTFLAGS="caf.exp"
(caf.exp is sufficient as the rest of the testsuite does not use
libgfortran/caf/single.c, which is the only thing your patch modifies.)


Otherwise, your patch looks OK.

Tobias,
who has currently no access to the GCC source code and can hence not
test himself.

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

[Daniel Carrera]

[-- 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;
 }
 
 

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

[Tobias Burnus]

Hi Daniel,

Daniel Carrera wrote:
>> 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.

Seemingly, there was also a bug in the single.c version:

+  local = malloc (size);
+  token = malloc (sizeof (void*) * 1);


I have added the missing line:
   token[0] = local;

Committed the patch with this change as Rev. 175124. Thanks for the patch!

Tobias

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

[Daniel Carrera]

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

Hello,

This is my second patch for GFortran. This patch adds a linked list to 
keep track of the allocated memory of all static coarrays and ensures 
that at the end of the program the memory is freed.

Additionally, I use MPI_Allgather in mpi.c to ensure that all images 
know the address of the stored memory in the other images. This will be 
needed when the images start sending data to each other.

There is no test case attached because there is no way to automatically 
test for allocated memory.

Here is my ChangeLog to go into libgfortran/ChangeLog:

2011-06-16  Daniel Carrera  <dcarrera@gmail.com>

	* caf/single.c (_gfortran_caf_register): Store the address
	of all static coarrays in a linked list.
	(_gfortran_caf_finalize): Free memory of staic coarrays.
	* caf/mpi.c (_gfortran_caf_register): Store the address
	of all static coarrays in a linked list. Initialize MPI
	if necessary.
	(_gfortran_caf_finalize): Free memory of staic coarrays.
	(_gfortran_caf_init): Check if MPI is already initialized
	before initializing again.
	* caf/libcaf.h: Add a type to caf_register_t to distinguish
	static coarrays and add the type caf_static_t to make the
	linked list of static coarrays.

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

[-- Attachment #2: caf-free-memory.patch --]
[-- Type: text/x-patch, Size: 4696 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,68 @@
 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;
+
+  local = malloc (size);
+  token = malloc (sizeof (void*) * caf_num_images);
+
+  /* Start MPI if not already started.  */
+  if (caf_num_images == 0)
+    _gfortran_caf_init (NULL, NULL, NULL, NULL);
+
+  /* token[img-1] is the address of the token in image "img".  */
+  MPI_Allgather (local,    sizeof (void*), MPI_BYTE,
+		 token[0], 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;
 }