[Patch, fortran] PR116733: Generic processing of assumed rank objects (f202y)
Paul Richard Thomas
paul.richard.thomas@gmail.com
Wed Oct 23 05:33:32 GMT 2024
Hi All,
Thanks to Thomas and Andre for the reviews. I believe that I have addressed
all of the concerns in the attached and will commit this afternoon if there
are no objections.
Best regards
Paul
On Tue, 24 Sept 2024 at 09:45, Andre Vehreschild <vehre@gmx.de> wrote:
> Hi Paul,
>
> in addition to Thomas' remarks (which I second to), I have the following:
>
> > diff --git a/gcc/fortran/intrinsic.cc b/gcc/fortran/intrinsic.cc
> > index 0a6be215825..d95f35145b5 100644
> > --- a/gcc/fortran/intrinsic.cc
> > +++ b/gcc/fortran/intrinsic.cc
> > @@ -293,11 +293,15 @@ do_ts29113_check (gfc_intrinsic_sym *specific,
> gfc_actual_arglist *arg)
> > &a->expr->where, gfc_current_intrinsic);
> > ok = false;
> > }
> > - else if (a->expr->rank == -1 && !specific->inquiry)
> > + else if (a->expr->rank == -1
> > + && !(specific->inquiry
> > + || (specific->id == GFC_ISYM_RESHAPE
> > + && (gfc_option.allow_std & GFC_STD_F202Y))))
> > {
> > gfc_error ("Assumed-rank argument at %L is only permitted as
> actual "
> > - "argument to intrinsic inquiry functions",
> > - &a->expr->where);
> > + "argument to intrinsic inquiry functions or to
> reshape. "
>
> Is it not a convention to write Fortran intrinsics function names all
> uppercase? I.e. RESHAPE when the function is meant just to make it clear
> like in
> the message above on C_LOC and PRESENT (lines 268--270).
>
> > + "The latter is an experimental F202y feature. Use "
> > + "-std=f202y to enable", &a->expr->where);
> > ok = false;
> > }
> > else if (a->expr->rank == -1 && arg != a)
> > @@ -307,6 +311,13 @@ do_ts29113_check (gfc_intrinsic_sym *specific,
> > gfc_actual_arglist *arg) &a->expr->where, gfc_current_intrinsic);
> > ok = false;
> > }
> > + else if (a->expr->rank == -1 && specific->id == GFC_ISYM_RESHAPE
> > + && !gfc_is_simply_contiguous (a->expr, true, false))
> > + {
> > + gfc_error ("Assumed rank argument to the reshape intrinsic at %L
> "
>
> Here, too?
>
> > + "must be contiguous", &a->expr->where);
> > + ok = false;
> > + }
> > }
> >
> > return ok;
>
> <snipp>
>
> > diff --git a/gcc/fortran/match.cc b/gcc/fortran/match.cc
> > index 0cd78a57a2f..81610b93345 100644
> > --- a/gcc/fortran/match.cc
> > +++ b/gcc/fortran/match.cc
> > @@ -1920,7 +1920,31 @@ gfc_match_associate (void)
> > gfc_association_list* a;
> >
> > /* Match the next association. */
> > - if (gfc_match (" %n =>", newAssoc->name) != MATCH_YES)
> > + if (gfc_match (" %n ", newAssoc->name) != MATCH_YES)
> > + {
> > + /* "Expected associate name at %C" would be better.
> > + Change associate_3.f03 to match. */
>
> That's an odd comment. Sounds to me like a remark to your self.
>
> > + gfc_error ("Expected associate name at %C");
> > + goto assocListError;
> > + }
> > +
> > + /* Required for an assumed rank target. */
> > + if (gfc_peek_char () == '(')
> > + {
> > + newAssoc->ar = gfc_get_array_ref ();
>
> This is not freeed in case of an error and may result in a memory leak,
> right?
>
> > + if (gfc_match_array_ref (newAssoc->ar, NULL, 0, 0) != MATCH_YES)
> > + {
> > + gfc_error ("Bad bounds remapping list at %C");
> > + goto assocListError;
> > + }
> > + }
> > +
> > + if (newAssoc->ar && !(gfc_option.allow_std & GFC_STD_F202Y))
> > + gfc_error_now ("The bounds remapping list at %C is an experimental
> "
> > + "F202y feature. Use std=f202y to enable");
> > +
> > + /* Match the next association. */
> > + if (gfc_match (" =>", newAssoc->name) != MATCH_YES)
> > {
> > gfc_error ("Expected association at %C");
> > goto assocListError;
>
> <snipp>
>
> > diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
> > index 07e28a9f7a8..aa0ee1b0164 100644
> > --- a/gcc/fortran/trans-expr.cc
> > +++ b/gcc/fortran/trans-expr.cc
>
> <snipp>
>
> > @@ -10784,6 +10815,13 @@ gfc_trans_pointer_assignment (gfc_expr * expr1,
> > gfc_expr * expr2)
> > gcc_assert (remap->u.ar.start[dim] &&
> > remap->u.ar.end[dim]);
> > + if (remap->u.ar.start[dim]->expr_type != EXPR_CONSTANT
> > + || remap->u.ar.start[dim]->expr_type !=
> EXPR_VARIABLE)
> > + gfc_resolve_expr (remap->u.ar.start[dim]);
> > + if (remap->u.ar.end[dim]->expr_type != EXPR_CONSTANT
> > + || remap->u.ar.end[dim]->expr_type != EXPR_VARIABLE)
> > + gfc_resolve_expr (remap->u.ar.end[dim]);
> > +
>
> Can't these resolves be done during resolve-stage? I have had some serious
> trouble with late resolves, therefore asking.
>
> > /* Convert declared bounds. */
> > gfc_init_se (&lower_se, NULL);
> > gfc_init_se (&upper_se, NULL);
>
> <snipp>
>
> > diff --git a/gcc/fortran/trans-stmt.cc b/gcc/fortran/trans-stmt.cc
> > index 86c54970475..450c11c06d7 100644
> > --- a/gcc/fortran/trans-stmt.cc
> > +++ b/gcc/fortran/trans-stmt.cc
> > @@ -1910,6 +1910,20 @@ trans_associate_var (gfc_symbol *sym,
> > gfc_wrapped_block *block) gfc_add_init_cleanup (block, gfc_finish_block
> > (&se.pre), tmp); }
> > /* Now all the other kinds of associate variable. */
> > + else if (e->rank == -1 && sym->attr.pointer && sym->assoc->ar)
> > + {
> > + gfc_expr *expr1 = gfc_lval_expr_from_sym (sym);
> > + gfc_free_ref_list (expr1->ref);
>
> What if sym.ts.type == BT_CLASS? I doubt this works in this case...
>
> > + expr1->ref = gfc_get_ref();
> > + expr1->ref->type = REF_ARRAY;
> > + expr1->ref->u.ar = *sym->assoc->ar;
> > + expr1->ref->u.ar.type = AR_SECTION;
> > + gfc_expr *expr2 = gfc_copy_expr (e);
> > + tmp = gfc_trans_pointer_assignment (expr1, expr2);
> > + gfc_add_init_cleanup (block, tmp, NULL);
> > + gfc_free_expr (expr1);
> > + gfc_free_expr (expr2);
> > + }
> > else if ((sym->attr.dimension || sym->attr.codimension) &&
> !class_target
> > && (sym->as->type == AS_DEFERRED || sym->assoc->variable))
> > {
>
> <snipp>
>
> > diff --git a/gcc/testsuite/gfortran.dg/f202y/f202y.exp
> b/gcc/testsuite/gfortran.dg/f202y/f202y.exp
> > new file mode 100644
> > index 00000000000..737a78937a7
> > --- /dev/null
> > +++ b/gcc/testsuite/gfortran.dg/f202y/f202y.exp
>
> <snipp>
>
> > +global gfortran_test_path
> > +global gfortran_aux_module_flags
> > +set gfortran_test_path $srcdir/$subdir
> > +set gfortran_aux_module_flags "-Werror -std=f2023"
>
> I would have bet, that it should be -std=f202y ...
>
> I mean, the directory says so. I get why you don't do it, but it is soooo
> counter-intuitive to me. I would put my f202y tests in here and would then
> be
> wondering why they don't work out of the box.
>
> <snipp>
>
> I hope those comments help at least a little bit. When you addressed them
> (in
> which way ever), the patch is ok for mainline from my side.
>
> Regards and thanks for the work,
> Andre
>
> On Mon, 23 Sep 2024 16:15:05 +0200
> Thomas Koenig <tkoenig@netcologne.de> wrote:
>
> > Am 23.09.24 um 11:02 schrieb Paul Richard Thomas:
> > > Hi All,
> > >
> > > The moment I saw the DIN4 proposal for "Generic processing of assumed
> > > rank objects", I thought that this was a highly intuitive and
> > > implementable proposal. I implemented a test version in June and had
> > > some correspondence with Reinhold Bader about it shortly before he
> > > passed away.
> > >
> > > Malcolm Cohen wrote J3/24-136r1 in response to this and I have posted
> a
> > > comment in PR116733 addressing the the extent to which the attached
> > > patch addresses his remarks.
> >
> > I think your approaches are sound.
> >
> > > Before this patch goes through the approval process, we have to
> consider
> > > how experimental F202y features can be carried forward. I was badly
> > > bitten by failing to synchronise the array descriptor reform branch to
> > > the extent that I gave up on it and adopted the simplified reform that
> > > is now in place. Given the likely timescale before the full adoption
> of
> > > the F202y standard, this is a considerable risk for
> > > experimental features, given the variability of active maintainers:
> > >
> >
> > That is correct. We (well, Nicolas) also saw the bit rot on the
> > native coarray branch.
> >
> > > What I propose is the following:
> > > (i) For audit purposes, I have opened PR116732, which should be
> blocked
> > > by PRs for each experimental F202y feature;
> >
> > I have added PR 116025 to this.
> >
> > > (ii) These PRs should represent a complete audit trail for each
> feature; and
> > > (iii) All such experimental features should be enabled on mainline by
> > > --std=f202y, which is equivalent to -std=f2023+f202y.
> >
> > As far as the -funsigned patch goes: I would like to keep the option
> > itself, but also enable it with -std=f202y.
> >
> > > The attached patch enables pointer assignment and associate, both with
> > > rank remapping, plus the reshape intrinsics. which was not part of the
> > > DIN4 proposal.
> > >
> > > The ChangeLog entries do a pretty complete job of describing the patch.
> > >
> > > Regtests correctly. OK for mainline?
> >
> > As a general remark: What we currently have are extensions, and we
> > should try to describe them so a user who is not familiar with the
> > J3 documents or our PRs and commit messages should be able to use
> > the features. I am certainly not there yet with the work on UNSIGNED,
> > but we should work on that so the documentation is fairly complete
> > when gcc15 is released.
> >
> > As for the patch itself: It looks good do me in principle. It still
> > needs some test cases (or did git omit these from your patch?
> > I've been bitten by that :-). One typo:
> >
> > + gfc_error ("The data-target at %L ia an assumed rank object and
> > so the "
> >
> > s/ia/is/
> >
> > So, OK in principle with reasonable test coverage, but if you could hold
> > for a few days before committing so others can also comment, that would
> > be good.
> >
> > Best regards
> >
> > Thomas
> >
>
>
> --
> Andre Vehreschild * Email: vehre ad gmx dot de
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://gcc.gnu.org/pipermail/fortran/attachments/20241023/9c93119c/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: resubmit.diff
Type: text/x-patch
Size: 45066 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/fortran/attachments/20241023/9c93119c/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Change.Logs
Type: application/octet-stream
Size: 3051 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/fortran/attachments/20241023/9c93119c/attachment-0001.obj>
More information about the Fortran
mailing list