This is the mail archive of the
fortran@gcc.gnu.org
mailing list for the GNU Fortran project.
Re: [PATCH] Cray Pointers
At 4:20 PM +0100 7/31/05, Paul Brook wrote:
On Friday 29 July 2005 23:27, Asher Langton wrote:
> Hi,
Here is a beta of the Cray pointer patch.
Some notes:
1) The flags are -fcray-pointer and -fbyte-type. fbyte-type is added
for compatibility with old LRLTRAN code. It enables a BYTE type that
is equivalent to INTEGER(1).
Please break this out and submit as a separate patch.
Okay.
> 2) The main things added are cray_pointee and cray_pointer bits to
the symbol_attribute, a pointer from pointee symbols to pointer
symbols, code to parse pointer statements, code to translate pointee
expressions and the loc intrinsic code. (The cray_pointer bit so far
has proved unnecessary, but I've left it in for now.) The pointee
translation is done entirely in the front end; that is, the pointees
disappear by the time the code is translated into GENERIC.
This sounds reasonable.
3) The attributes of pointees are not thoroughly checked. This won't
be hard to add, but I need to figure out which attributes might be in
conflict. So far, I've been mostly concerned with compiling our
existing (valid) code.
I think you need to do this before we can accept the patch. At least the
documentation/design, if not the actual implementation.
I'll work on adding this code tomorrow.
> 4) The implementation allows a user to explicitly declare the type
and kind of a pointer. For example:
integer ipt
pointer (ipt, pointee)
However, this code isn't portable. It is safer to omit the 'integer
ipt' line and let the pointer statement declare ipt to have
kind=gfc_index_integer_kind. On Itaniums and Opterons, where
sizeof(int) != sizeof(void *), this code requires -fdefault-integer-8
to run correctly. We could force the kind of the pointer to match
> the hardware pointer size, but for now I leave that up to the user.
This probably warrants a warning if the declared type is smaller than a
pointer.
Yes, that's a good idea. I'll add that.
5) I don't yet have any tests included in the patch. I'll try to
figure out the dejagnu framework today. We have a nice Cray pointer
testsuite here at LLNL, but I can't release it yet; we're trying to
get it GPL'd, though. In its place, I've attached a test program
that hits all of the problems we found during internal testing. It's
pretty long, but I'll try to convert it into something useable for
the test suite. (Regarding other Fortran compilers: ifort passes
this test as is; xlf90 and pathscale pass a version without the
derived type pointees; and pgf90 fails, probably due to aggressive
optimization.)
We will need tests before the patch is applied.
Some general comments:
We need user level documentation. This needs to describe the syntax and
semantics of cray pointers, the LOC intrinsics and they interact with other
features. In particular dummy variables as pointees, f90 pointer types and
assumed shape arrays have non-obvious semantics if they are permitted.
What are the aliasing rules for cray pointers, especially given your problems
getting your original testcase to work reliably? Are the pointers byte or
type addressable?
I've written some documentation already, and I'll add a section about
the aliasing and the other issues you've mentioned.
Some of this documentation is required so we can tell if the implementation is
performing as expected. Some indication of where this extension (the
specification, not the implementation) comes from would also be good.
Some of the code isn't formatted properly. In particular curly braces
("{", "}") are in the wrong place, and some lines are >80 characters long.
I'll clean this up. (Is there an emacs indentation mode that I
should be using?)
> + static match
+ cray_pointer_decl (void)
...
+ /* Need to add error checking here */
Missing features should be annotated with TODO: so it's obvious they are not
deliberate omissions.
+ /* 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. */
I'm not sure "force the type" is correct here. Maybe "override the implicit
type" is more appropriate.
if (gfc_match_char (')') != MATCH_YES)
+ {
+ gfc_error ("Unexpected character in variable list at %C");
"Expected ")" at %C" would be a better error. The existing messages is
particularly confusing because "CHARACTER" has meaning in fortran.
+ /* Is MATCH_ERROR a possibility here? */
+ if (gfc_match_char (',') == MATCH_YES)
+ {
+ if (gfc_match_char ('(') != MATCH_YES)
+ {
+ gfc_error ("Expected '(' at %C");
+ return MATCH_ERROR;
+ }
+ if (cray_pointer_decl() != MATCH_YES) /* recursively get next decl
*/
+ return MATCH_ERROR;
+ }
The code is probably better moved to a loop in gfc_match_cray_pointer. That
way you can avoid the recursion.
+ if (gfc_match_eos () != MATCH_YES)
+ {
+ gfc_error ("Unexpected character in variable list at %C");
We're expecting "," or end of statement. Helpful error messages are good :-)
I'll make these changes. I've spent most of my time recently on
translating pointees; I'll go back and clean up the parsing code now.
> + /* Cray Pointees can be declared as:
+ pointer(ipt,a(n,m,...,*)
The second line appears to be incomplete
It's not incomplete, but it's obviously not clear. I was referring
to pointees being declared as assumed size arrays.
> + /* Intrinsic function for Cray Pointers */
+ if (gfc_option.flag_cray_pointer)
+ {
> + add_sym_1("loc", 0, 1, BT_INTEGER, ii, GFC_STD_GNU,
This should not be conditional on -fcray-pointers. g77 has LOC(),
so it's obviously useful even without > gray pointers.
Okay, I'll change this.
> ! if (gfc_option.flag_cray_pointer) /* Need braces, otherwise
ambiguous */
This comment should be moved inside the conditional, otherwise it looks like
it applies to the if. Probably also worth explicitly stating that it's
ambiguous with f95 pointer type declarations.
Actually, I was referring to the if: the match() routine is a macro:
#define match(keyword, subr, st) \
if (match_word(keyword, subr, &old_locus) == MATCH_YES) \
return st; \
else \
undo_new_statement ();
The curly braces are necessary around the if. I'll clarify the
comment in the code, though.
> *************** gfc_conv_array_parameter (gfc_se * se, g
*** 3916,3922 ****
&& expr->ref->u.ar.type == AR_FULL && g77)
{
sym = expr->symtree->n.sym;
! tmp = gfc_get_symbol_decl (sym);
if (sym->ts.type == BT_CHARACTER)
se->string_length = sym->ts.cl->backend_decl;
if (!sym->attr.pointer && sym->as->type != AS_ASSUMED_SHAPE
--- 3925,3944 ----
&& expr->ref->u.ar.type == AR_FULL && g77)
{
sym = expr->symtree->n.sym;
!
! /* check to see if we're dealing with a Cray Pointee */
! if (sym->attr.cray_pointee)
! {
! tree pte_decl = gfc_get_symbol_decl(sym);
! tmp = gfc_get_symbol_decl(sym->cp_pointer);
! if (sym->cp_pointer->attr.dummy)
! tmp = gfc_build_indirect_ref(tmp);
! tmp = convert(build_pointer_type(TREE_TYPE(pte_decl)),tmp);
! }
! else
This code should probably be moved into gfc_get_symbol_decl.
*************** 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;
This shouldn't be necessary. We shouldn't be creating the decl in the first
place. We may want to do something for debug info, but this almost certainly
isn't it.
diff -c -3 -p -r1.54 trans-expr.c
*** trans-expr.c 20 Jul 2005 01:19:33 -0000 1.54
--- trans-expr.c 29 Jul 2005 18:17:47 -0000
*************** gfc_conv_variable (gfc_se * se, gfc_expr
*** 312,318 ****
--- 312,338 ----
tree se_expr = NULL_TREE;
se->expr = gfc_get_symbol_decl (sym);
+
+ /* Handle Cray Pointees */
+ if (sym->attr.cray_pointee)
+ {
+ tree tmp = gfc_get_symbol_decl(expr->symtree->n.sym->cp_pointer);
+ if (expr->symtree->n.sym->cp_pointer->attr.dummy) /* parameters
need to be dereferenced */
+ tmp = gfc_build_indirect_ref(tmp);
+ if (expr->ref && expr->ref->type == REF_ARRAY
+ && TREE_CODE(TREE_TYPE(se->expr)) == POINTER_TYPE)
+ {
+ se->expr = convert(TREE_TYPE(se->expr), tmp);
+ }
+ else
+ {
+ tmp = convert(build_pointer_type(TREE_TYPE(se->expr)),tmp);
+ se->expr = gfc_build_indirect_ref (tmp);
+ }
As above. Should be handled by gfc_get_symbol_decl.
Okay.
> + /* gfc_add_block_to_block (&se->pre, &argse.pre); */
+ /* gfc_add_block_to_block (&se->post, &argse.post); */
Don't leave commented blocks of code. Either delete it or replace it with a
proper comment.
+ se->expr = convert (gfc_unsigned_type(long_integer_type_node),
se->expr);
+
+ /* Create a temporary variable for loc return value. We need to make
sure
+ * that the result isn't cast to the wrong integer kind along the
+ * way. (This was an issue on Alphas and Itaniums.) We assume
+ * that sizeof (void *) <= size (long int) */
+ temp_var = gfc_create_var(gfc_unsigned_type(long_integer_type_node),
NULL);
+ gfc_add_modify_expr (&se->pre, temp_var, se->expr);
+ se->expr = temp_var;
The LOC expression should already have been resolved to the correct type. Use
that.
I was running into problems when I didn't explicitly create a
temporary variable. I'll work on this one some more.
> + break;
+ case GFC_ISYM_LOC:
+ gfc_conv_intrinsic_loc(se, expr);
break;
+
Blank line in the wrong place.
Paul
Thanks for the thorough review, Paul. I'll have new version of the
patch ready in a day or so.
-Asher