Re: [Patch, Fortran] First ASSOCIATE patch and some questions / RFCs

[Daniel Kraft <d at domob dot eu>]

Hi,

thanks for the prompt review and reply!

Tobias Burnus wrote:
> On 06/06/2010 07:47 PM, Daniel Kraft wrote:
> > Association of names to variables (currently only expressions).  I
> > think that my original idea of replacing each occurrence directly in
> > the parser with the corresponding gfc_expr does not work, because it
> > will do the wrong thing if the selector expression changes, as in
> > these two cases:
> >
> > INTEGER, POINTER :: ptr
> > ptr => something
> > ASSOCIATE (x => ptr)
> >   ptr => something else
> >   ! x should still refer to something
> > END ASSOCIATE
>
> I think that's not the case: "x" is just an associated name for "ptr"
> and not a pointer, which then points to the target of "ptr" (i.e.
> "something" in your example). That's also what I get with ifort.

"16.5.1.6 Construct association:

...

3) If the selector has the POINTER attribute, it shall be associated; 
the associate name is associated with the target of the poiner and dos 
not have the POINTER attribute.

..."

This is what made me understand my example the way I did.  BTW, there's 
also a similar clause for ALLOCATABLE there, so an associate name should 
also refer to the actual allocation in case it is moved to another 
variable or the like (instead of the original variable), as I read it.

> > INTEGER :: n
> > REAL :: array(10)
> > n = 2
> > ASSOCIATE (arr => array(n : n+2)
> >   n = 5
> >   ! arr is still array(2 : 4)
> > END ASSOCIATE
>
> Here, the standard explicitly mentions (in an informal note) that that
> the bounds have to be evaluated first, i.e. arr is - as you wrote -
> still array(2:4):
>
> "NOTE 16.11  The association between the associate name and a data
> object is established prior to execution of the block and is not aected
> by subsequent changes to variables that were used in subscripts or
> substring ranges in the selector."
>
> Thus, it could make sense to evaluate the bonds before and save them in
> a local integer variable but still do the
> associate-name--to--data-object replacement - but using the local
> integer. That way one reduces the pointer variables one presents to the
> middle end and gets closer to the semantics of the Fortran standard.

That's an interesting idea, although it seems rather complicated and 
somewhat hackish to me -- however, if my reading of the standard above 
is correct, this unfortunately does not help for the pointers.

> > So instead we need another strategy; possibly using a BLOCK local
> > pointer that it pointed to the selector.
>
> That would be an alternative, though I wonder whether just having a
> local bounds variable would be better - especially in the case of
> compile-time constants, the resulting code could be cleaner.

Unfortunately, it seems that the "has not the POINTER attribute" from 
the quote above actually forbids this implementation; do you know what 
consequences this attribute has and if we could easily circumvent that? 
 Say by some internal flag that a variable is a pointer only internally 
but not from the user code's point of view?

> > Does this provoke problems, when the selector is not TARGET or the like?
> Well, TARGET is a Fortran concept and not a GIMPLE concept; I think one
> has to think carefully which item to mark as TYPE_QUAL_RESTRICT and
> which not - and of course, the more TYPE_QUAL_RESTRICT the better. Best
> ask a middle-end person when it is clear how you use it.

Yes of course, but I'm generating in some sense "high-level Fortran", 
don't I?  So I was not sure whether it is possible to introduce a 
POINTER in gfortran's internal language tree where the target is not 
TARGET.  But I guess (and hope) that it actually is possible.

> > Association to array expressions.  The problem here is that for
> > something like:
> >
> > INTEGER :: array(10)
> > ASSOCIATE (doubled => 2 * array)
> >   PRINT *, doubled(2)
> > END ASSOCIATE
> >
> > During parsing, the expression "2 * array" seems not to have its rank
> > defined yet; this is only done at resolution stage.  However, when
> > parsing doubled(2), the compiler already needs to know that doubled is
> > an array!  Any ideas what we could do here?
>
> I do not have an ad-hoc idea; I think one has to defer this to
> resolution time as I do not see how you want to handle
>   doubled => 2 * array(f())
> where "f" is a generic procedure which either returns a scalar integer
> (-> doubled is a scalar) or it returns an array (-> doubled is an array
> of rank).

Yes -- but when I try this currently, the parser does not accept 
"doubled(2)" because it does not know doubled is an array; and this 
happens at parsing time.  So I think the only way around is to allow 
subscripts in the parser for all associate names at first and only check 
for them being arrays later during resolution.  Does this introduce 
problems, with for instance recognition of function calls vs. array 
subscripts?

> +      /* The target is a variable (and may be used as lvalue) if it's an
> +	 EXPR_VARIABLE and does not have vector-subscripts.  In addition,
> +	 it must not be coindexed.  */
> +      newAssoc->variable = (newAssoc->target->expr_type == EXPR_VARIABLE
> +			    && !gfc_is_coindexed (newAssoc->target)
> +			    && !gfc_has_vector_subscript (newAssoc->target));
>
>
> I think you got the coarray check wrong. At least I read C803 such that
> coarrays are not allowed at all - contrary to vector-subscripted
> variables, which are only allowed in a variable-definition context 
(C801).

I agree.  I'll change this is_conindexed check into an unconditional error.

> -  /* Check for the IF, DO, SELECT, WHERE, FORALL, CRITICAL, and BLOCK
> +  /* Check for the IF, DO, SELECT, WHERE, FORALL, BLOCK and ASSOCIATE
>       statements, which might begin with a block label.  The match 
functions for
>
> What happened to "CRITICAL" in the comment?

I don't know, but I'll add it back of course.

> +  my_ns = gfc_build_block_ns (gfc_current_ns); [...]
>
> +  new_st.ext.block.ns = my_ns;
> [...]
>
> +	if (gfc_get_sym_tree (a->name, NULL, &a->st, false))
> +	  /* XXX: Hopefully.  */
> +	  gcc_unreachable ();
>
>
> I think the unreachable is fine. You create a new namespace and thus
> there should be no non-zero return value (= ambiguious symbol) as there
> should be no other symbols in this namespace.

Ok.

> @@ -3543,7 +3638,6 @@ parse_executable (gfc_statement st)
>  	  case ST_CRITICAL:
> -	  case ST_BLOCK:
>  	  case ST_FORALL:
>  	  case ST_WHERE:
>  	  case ST_SELECT_CASE:
> @@ -3573,6 +3667,10 @@ parse_executable (gfc_statement st)
> +	case ST_ASSOCIATE:
> +	  parse_associate ();
> +	  break;
> +
>
>
> Why don't you need ST_BLOCK anymore?

Hm, I thought that I added this ST_BLOCK there by mistake and tried 
removing it; but it seems you did add this.  That's probably why I did 
not see what it is for -- do we need ST_ASSOCIATE there then, too?

> I miss a test case for the following two errors messages:
>
> associate (x => 5)
>   integer :: u ! { dg-error "Unexpected data declaration statement" }
>   x =  34 ! { dg-error "can't appear in a variable definition context" }
> end associate
> end

The second one is in associate_3.f03, but the first is a good idea to 
add.  I'll do that.

Regarding -fdump-parse-tree:  You're right that this is currently 
missing for both ASSOCIATE and BLOCK; my plan was/is to do this as well 
as gfc-internals for them in the very next patch if that is ok for you. 
 Otherwise I can add this also to this one, of course.

> Otherwise, the patch looks good.

Thanks.  I'll update it right now and re-submit (but with only the 
changes you suggested, so you probably do not have to re-review it). 
AFAIK, the trunk is frozen until tomorrow anyways, right?

Thanks,
Daniel

--
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Kni-Mon-Pri