This is the mail archive of the fortran@gcc.gnu.org mailing list for the GNU Fortran project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: First Patch


Hi Tobias,

Ok, I'll prepare a new patch and then write again to this list and CC to gcc-patches@... with an appropriate email subject. Do I need to subscribe to gcc-patches@... first?


Thirdly, you miss a changelog entry. It should be not a diff (as
ChangeLog will likely change.) Note that the indention is via a
tab and that there are two spaces between date and name and between
name and email address. See gcc/fortran/ChangeLog and
gcc/testsuite/ChangeLog for examples. The changelog should contain
what has been changed - not why. Cf. also
http://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html

Ok. I've read the instructions on Changelog and I came up with this:


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

	* mpi.c: Changed _gfortran_caf_sync_all and _gfortran_caf_sync_images
    moving the status to the parameter list.
	* single.c: Changed the declaration of _gfortran_caf_sync_all
    and _gfortran_caf_sync_images.
	* libcaf.h: Changed the declaration of _gfortran_caf_sync_all
    and _gfortran_caf_sync_images.
	* trans-decl.c: Updated declaration of caf_sync_all and caf_sync_images
	* trans-stmt.c: Changed gfc_trans_sync to handle a "stat" variable that
	has an integer type different from integer_type_node.



Let me know if that's ok.


Coding style: I think you miss several tabs: Coding style is to replace
spaces by a tab if possible (8 spaces = tab).

Ok. My editor was set to use spaces. I just changed the settings.





Regarding:
+          casting it to the correct type.
+        */
The */ should be in the preceeding line, i.e. "... type.  */" with two
spaces before the "*/".


Ok.


+       if (stat == null_pointer_node || TREE_TYPE(stat) == integer_type_node)
+            tmp = build_call_expr_loc( input_location, gfor_fndecl_caf_sync_all,

Coding style is to place a space before the "(" and none after it, i.e.
"TREE_TYPE (stat)" and "..._expr_loc (input_...".

Ok.





      }
-  else
+
+   else
      {

The "else" looks wrongly indented.


I looked into this. The root issue is that my text editor was indenting with three spaces after an open { bracket, so actually a lot of lines were wrongly indented by 1 or 2 spaces. I just fixed my editor and fixed the lines.



+ 5, integer_type_node, integer_type_node, pint_type, build_pointer_type (pchar_type_node),

That line is too long. Additionally, you need a pointer to an integer
("int *") and not an integer ("int") as argument. Thus, you should use
"pint_type" instead of "integer_type_node".

Ok, fixed.



As a matter of taste, I'd would prefer for SYNC IMAGES:
_gfortran_caf_sync_images (int count, int images[], int *status,
                            char *errmsg, int errmsg_len)
instead of
_gfortran_caf_sync_images (int *status, int count, int images[],
                            char *errmsg, int errmsg_len)
to be more in line with the standard ("SYNC IMAGES (imageset, stat-list)")
and to place optional values after the required ones.

Ok, fixed. I also updated trans-decl.c to this:



gfor_fndecl_caf_sync_images = gfc_build_library_function_decl_with_spec (
get_identifier (PREFIX("caf_sync_images")), ".RRWW", void_type_node,
5, pint_type, integer_type_node, pint_type,
build_pointer_type (pchar_type_node), integer_type_node);



+_gfortran_caf_sync_all (int *status, char *errmsg, int errmsg_len)
  {
-  int ierr;
-  ierr = MPI_Barrier (MPI_COMM_WORLD);
+  status = MPI_Barrier (MPI_COMM_WORLD);

There are two issues:
a) You don't want to do a pointer assignment but you want to
    assign the value, i.e.:  *status = MPI_...
b) If the argument is absent - i.e. it is a NULL pointer -, one
    cannot assign to the variable.
The easiest to probably to keep ierr - and later set stat via
   if (state)
     state = ierr;

Could you also change "status" into "stat"? I know that I used "status",
but I think it makes sense to follow the standard and use "stat".

Ok. Now I have this:


  ierr = MPI_Barrier (MPI_COMM_WORLD);
  if (stat)
    *stat = ierr;


The error diagnostic should be as follows:
- If no error occurs - and "stat" is not present, don't do anything
   special
- If no error occurs - and "stat" is present (not NULL), assign 0 to it.
- If an error occurs - and "stat" is not present, print an error and exit
- If an error occurs - and "stat" is present, assign the error code to it
   - and if "errmsg" is present, set it.
(One could also defer this part.)

Ok, how about this:


void
_gfortran_caf_sync_all (int *stat, char *errmsg, int errmsg_len)
{
  int ierr = MPI_Barrier (MPI_COMM_WORLD);

  if (stat && !ierr)
    *stat = 0

  if (ierr)
    {
      const char msg[] = "SYNC ALL failed";
      if (errmsg_len > 0)
	{
	  int len = ((int) sizeof (msg) > errmsg_len) ? errmsg_len
	    : (int) sizeof (msg);
	  memcpy (errmsg, msg, len);
	  if (errmsg_len > len)
	    memset (&errmsg[len], ' ', errmsg_len-len);
	}
      if (stat)
	*stat = ierr;
      else
	{
	  fprintf(stderr,"SYNC ALL failed\n");
	  exit(ierr);
	}
    }
}


The error handling is based on the current _gfortran_caf_sync_all. If it looks good, I'll do the same for _gfortran_caf_sync_images.




Seemingly, we need a test case for SYNC ALL/IMAGES - with and without stat=
argument. For the argument version, one should set "stat" to some value and
check afterwards that it is indeed 0.

The test case has to be placed into gcc/testsuite/gfortran.dg/coarray/; if
it is in that directory, it is automatically tested with both
-fcoarray=single and with -fcoarray=lib. I think some test case
gcc/testsuite/gfortran.dg/coarray_*.f90 could be used as template. The test
has to be "! { dg-do run }" as the library has to be checked.

Ok. I'll take a look at how the tests work. Should the test be added to this patch or should it be a new patch?


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]