[PATCH] Cray Pointers (slight modifications)

[Tobias Schlueter]

(I'm sorry, I still haven't found a way to thread messages correctly, when 
mailing from the office -- pine explicitly disallows modifying the 
In-Reply-To header)

I've had a first look over the Cray pointers patch, and I'm hoping to 
review it fully over the next few days.  So far it looks good, but I have 
a few remarks already.

First, do we really want to allow this extension in cases that are not 
Fortran 77 + Cray pointers, IOW pointees of user-defined types?

There are a number of formatting issues that I ran into, I'll just 
highlight the various kinds of issues:

> + try
> + gfc_check_loc (gfc_expr *expr)
> + {
> +    return variable_check(expr,0);
> + }
> + 

Wrong level of indentation (3 spaces instead of 2).

> --- 899,914 ----
>     char name[GFC_MAX_SYMBOL_LEN + 1];
>     gfc_expr *initializer, *char_len;
>     gfc_array_spec *as;
> +   gfc_array_spec *cp_as; /* Extra copy for Cray Pointees */

Comments should end in punctuation mark plus two spaces.  (In order to 
make sure that emacs senctence commands work.  Sorry, vi users :-)  They 
should also begin with a blank following the asterisk, followed by a 
capital letter.

> --- 921,929 ----
>   
>     /* Now we could see the optional array spec. or character length.  
> */
>     m = gfc_match_array_spec (&as);
> !   if (gfc_option.flag_cray_pointer && m == MATCH_YES)
> !      cp_as = gfc_copy_array_spec(as); /* extra copy for Cray pointees 
> */
> !   else if (m == MATCH_ERROR)

Here you have both of the previous combined :-)

> + 		  gfc_error("Duplicate array spec for Cray pointee at 
                           ^
> %C.");
> + 		  gfc_free_array_spec (cp_as);
> + 		  m = MATCH_ERROR;
> + 		  goto cleanup;
> + 		}
> + 	      else
> + 		{
> + 		  if (gfc_set_array_spec (sym, cp_as, &var_locus) == 
> FAILURE)
> + 		    gfc_internal_error("Couldn't set pointee array 
                                      ^
> spec.");
> + 	      
> + 		  /* Fix the array spec */
> + 		  m = gfc_mod_pointee_as(sym->as);  
                                        ^

Missing blanks.  (Again, the comment is not formatted correctly.)

+     }    /* End Cray Pointee handling */

We don't do these kind of comments, but I can't find anything in the 
coding standards forbidding them, and I'm not opposed to this.

> +    This routine recursively deals with multiple declarations in a 
> single
> +    statement. */
> + static match
> + cray_pointer_decl (void)

Please leave a blank line between the function and its comment.

> +   /* Add pointer to symbol table */
> +   var_locus = gfc_current_locus;
> +   gfc_clear_attr (&current_attr);
> +   gfc_add_cray_pointer (&current_attr, &var_locus);
> +   current_ts.type=BT_INTEGER;
                     ^
> +   current_ts.kind=gfc_index_integer_kind;
> +   as = NULL; 
> +    
> +   /* first, see if pointer already exists.  If it does, and it was 
> first
> +      seen as a dummy argument, we might need to force the type. */
> +   gfc_find_symbol(ptr_name,gfc_current_ns,1,&cptr);
                     ^         ^              ^ ^
Please leave blanks before / after "=" and other other operators, and 
please also after ",".

> +   else if (build_sym (ptr_name, NULL, &as, &var_locus)==FAILURE)
> +     {
> +       gfc_error("Cray Pointer name declaration error at %C");
> +       return MATCH_ERROR;
> +     }

I wouldn't know what this error message means, if I couldn't look at the 
code.

As for the way, you add the symbols to the symbol table, is there a reason 
you're not using gfc_match_symbol, like, say, do_parm (whose function 
is fairly similar) does?  I couldn't think of a reason not to do so 
offhand.

> + gfc_match_cray_pointer (void)
> + {
> +   match m = cray_pointer_decl ();
> +   /* Check to see if there are more declarations */
> +   while (m != MATCH_ERROR 
> + 	 && gfc_match_char (',') == MATCH_YES)
> +     {
> +       if (gfc_match_char ('(') != MATCH_YES)

Please leave a blank line between variable declarations and the function's 
code.  Also, please check for the opening parenthesis inside the matcher 
function, the same way this is handled for the parameter statement.

> +    if (sym->attr.cray_pointee
> +        && lvalue->ref != NULL
> +        && lvalue->ref->u.ar.type == AR_FULL
> +        && lvalue->ref->u.ar.as->cp_was_assumed)
> +    {
> +       gfc_error ("Illegal assignment to assumed-size Cray Pointee at 
> %L",
> + 		 &lvalue->where);
> +       return FAILURE;
> +    }

Is this allowed for, say, a(1:), where a is an assumed-size craz pointee?  
I wouldn't think so.

> --- 734,743 ----
>     struct gfc_namespace *ns;	/* namespace containing this symbol */
>   
>     tree backend_decl;
> +   
> +     
> +   /* Defined only for Cray PointEES; points to their pointer */
> +   struct gfc_symbol *cp_pointer;
>   }
>  gfc_symbol;
 
Please put this after components field, that place seems more suited.

> + @smallexample
> + mymalloc.c:
> + 
> +         void mymalloc_(ptr,nbytes)
> +         int **ptr, *nbytes;
> +         @{
> +             *ptr = (int *) malloc(*nbytes);
> +             return;
> +         @}

The documentation looks very good, but can you please make this ANSI C.

> + unsafe to use iarr and target simultaneously.  This is not unique to
> + the gfortran implementation; every known implementation of Cray
> + pointers will produce incorrect results when optimizing code such as
> + the following:

Just pointing this out is IMO sufficient -- in Fortran 77 nothing could 
ever alias, so this doesn't seem to be out of place.

(NB, calling a subroutine as CALL s(a,a) would yield undefined results if 
s modified its dummy arguments.  Fortran 90 adds the TARGET attribute to 
indicate stuff that may be aliased, but the restriction on dummy arguments 
is maintained)

> + applied to Cray pointers and pointees.  Pointees may not have the
> + attributes ALLOCATABLE, INTENT, OPTIONAL, DUMMY, TARGET, EXTERNAL,
> + INTRINSIC, or POINTER.  Pointers may not have the attributes
> + DIMENSION, POINTER (Fortran 90 Pointer), TARGET, ALLOCATABLE,
> + EXTERNAL, or INTRINSIC.  Pointees may not occur in more than one

If you think it's beneficial to explain that POINTER refers to Fortran 90 
pointers, please do so in the first place they're referenced.

One thing which not became clear to me from the documentation:
say,
POINTER (p,d)
DOUBLE PRECISION d, e(5)

p = loc(e(1)) + 1

Will this point to e(2)?  The portland group compiler and your 
implementation think otherwise (they will make p point to the byte 
following e(1) instead of e(2)), which I wouldn't expect form the 
documentation I've read in other places.  From what I've read, I would 
expect this to make p point either to the byte 4 bytes behind e(1) or to 
e(2) (which lies 8 bytes after e(1)).  If the convention is really to use 
byte-sized increments, the documentation should definitely make this 
explicit.

> *** 247,253 ****
>       case 'p':
>         match ("print", gfc_match_print, ST_WRITE);
>         match ("parameter", gfc_match_parameter, ST_PARAMETER);
> !       match ("pause", gfc_match_pause, ST_PAUSE);
>         match ("pointer", gfc_match_pointer, ST_ATTR_DECL);
>         if (gfc_match_private (&st) == MATCH_YES)
>   	return st;
> --- 249,257 ----
>       case 'p':
>         match ("print", gfc_match_print, ST_WRITE);
>         match ("parameter", gfc_match_parameter, ST_PARAMETER);
> !       match ("pause", gfc_match_pause, ST_PAUSE);      
> !       if (gfc_option.flag_cray_pointer)
> ! 	 match("pointer ( ", gfc_match_cray_pointer, ST_ATTR_DECL);      
>         match ("pointer", gfc_match_pointer, ST_ATTR_DECL);
>         if (gfc_match_private (&st) == MATCH_YES)
>  	return st;

You could move the if (gfc_option.flag_cray_pointer) check into 
gfc_match_cray_pointer.  This would allow issuing a more informative 
diagnostic than "Syntax error".

> +   /* Skip checks for Cray Pointee arrays */
> +   if (as->cray_pointee)
> +       return SUCCESS;
> + 

Please move that check into compare_spec_to_ref (or maybe even 
resolve_array_ref?  That way you could also do away with setting the fake 
value for the upper bound in the highest dimension, I haven't yet checked 
which checks are needed, and I want to finish this first run, before I 
have to return to my polylogarithms).

> *** 3941,3947 ****
>     gfc_conv_expr_descriptor (se, expr, ss);
>   
>     if (g77)
> !     {
>         desc = se->expr;
>         /* Repack the array.  */
>         tmp = gfc_chainon_list (NULL_TREE, desc);
> --- 3956,3962 ----
>     gfc_conv_expr_descriptor (se, expr, ss);
>   
>     if (g77)
> !   {  
>         desc = se->expr;
>         /* Repack the array.  */
>         tmp = gfc_chainon_list (NULL_TREE, desc);
...
> *************** gfc_finish_var_decl (tree decl, gfc_symb
> *** 407,413 ****
>        This is the equivalent of the TARGET variables.
>        We also need to set this if the variable is passed by reference 
> in a
>        CALL statement.  */
> !   if (sym->attr.target)
>       TREE_ADDRESSABLE (decl) = 1;
>     /* If it wasn't used we wouldn't be getting it.  */
>     TREE_USED (decl) = 1;
> --- 407,418 ----
>        This is the equivalent of the TARGET variables.
>        We also need to set this if the variable is passed by reference 
> in a
>        CALL statement.  */
> ! 
> !    /* We don't want real declarations for Cray Pointees */
> !    if (sym->attr.cray_pointee)
> !      return;
> !      
> !    if (sym->attr.target)
>       TREE_ADDRESSABLE (decl) = 1;
>    /* If it wasn't used we wouldn't be getting it.  */
>     TREE_USED (decl) = 1;

Unintentional whitespace changes lurk here.

I'll try to get into the depths of your code by the weekend, and if you 
address the issues I'm pointing out, I'll have an easier time finishing my 
review.

Thanks,
- Tobi

:REVIEWURL http://gcc.gnu.org/ml/fortran/2005-08/msg00180.html: