Hi Christophe,
As far as I can tell, the problem with reduce_1.f90 is restricted to one
call to the scalar valued version of the new intrinsic function. When the
result is array valued, all seems to be well.
I would be grateful if you would apply the attached patch and let me know
if the problem goes away for you. In either case, I will seek Jakub
Jelnik's help because I have completely run out of ideas.
Je vous en remercie d'avance.
Paul
On Wed, 2 Apr 2025 at 10:12, Christophe Lyon <christophe.l...@linaro.org>
wrote:
> Hi!
>
> On Wed, 19 Mar 2025 at 19:22, Paul Richard Thomas
> <paul.richard.tho...@gmail.com> wrote:
> >
> > Hi Andre,
> >
> > Thanks for the review - I'll act on the points that you raised.
> >
> > The Linaro people reported a failure in reduce_1.f90 execution, which I
> believe is due to incorrect casting of 'dim' and a wrong specification of
> its kind. I am waiting to hear back from them as to whether or not I have
> fixed the failure.
> >
>
> Sorry I notice this message just today, so it's a bit outdated...
> I've looked at bugzilla, so I've noticed that the are proper bug
> reports about this now (and I've just checked, the problem is still
> present on arm).
>
> When you say you are "waiting to hear back from them as to whether or
> not I have fixed the failure", did you contact us directly? (I'm
> trying to understand if we missed your message, or how we could
> improve communication).
>
> Thanks,
>
> Christophe
>
>
>
> > Cheers
> >
> > Paul
> >
> >
> > On Wed, 19 Mar 2025 at 12:39, Andre Vehreschild <ve...@gmx.de> wrote:
> >>
> >> Hi Paul,
> >>
> >> I took a look at your patch and think I found some improvements needed.
> In
> >>
> >> +bool
> >> +gfc_check_reduce (gfc_expr *array, gfc_expr *operation, gfc_expr *dim,
> >> + gfc_expr *mask, gfc_expr *identity, gfc_expr *ordered)
> >> +{
> >>
> >> ...
> >>
> >> + if (formal->sym->attr.allocatable || formal->sym->attr.allocatable
> >> + || formal->sym->attr.pointer || formal->sym->attr.pointer
> >> + || formal->sym->attr.optional || formal->sym->attr.optional
> >> + || formal->sym->ts.type == BT_CLASS || formal->sym->ts.type ==
> BT_CLASS)
> >> + {
> >> + gfc_error ("Each argument of OPERATION at %L shall be a scalar, "
> >> + "non-allocatable, non-pointer, non-polymorphic and "
> >> + "nonoptional", &operation->where);
> >> + return false;
> >> + }
> >>
> >> The if is only looking at the first formal argument. The right-hand
> sides
> >> of the || miss a ->next-> to look at the second formal argument, right?
> >>
> >> May be you also want to extend the tests!?
> >>
> >> Without having looked at it, but can't you extract the whole block of
> >>
> >> + if (array->ts.type == BT_CHARACTER)
> >> + {
> >> + unsigned long actual_size, formal_size1, formal_size2,
> result_size;
> >> ...
> >> + return false;
> >> + }
> >> + }
> >>
> >> and share it with the checks for co_reduce? I figure way to many DRY
> principle
> >> violations are in gfortran. So when we can start this, why not do it?
> And a
> >> call to a routine, like check_char_arg_conformance() speaks way better,
> then
> >> having to read all that code ;-)
> >>
> >> In gfc_resolve_reduce() identity and ordered are marked as UNUSED.
> Should these
> >> not a least be resolved?
> >>
> >> Please run contrib/check_GNU_style on your patch. It reports several
> issues
> >> (haven't look into their validity).
> >>
> >> In the Changelog:
> >>
> >> - (gfc_check_rename): Add prototype for intrinsic with 6
> arguments.
> >> + * gfortran.h: Add prototype for intrinsic with 6 arguments.
> >>
> >> s/discription/description/
> >>
> >> I also encountered that nit with the executable stack when working in
> >> OpenCoarrays, but haven't had time (or desire) to look into it. I will
> put
> >> myself into CC of the pr Jerry mentioned.
> >>
> >> Besides the mentions above, this looks good to me.
> >>
> >> Thanks for the patch and
> >>
> >> Regards,
> >> Andre
> >>
> >>
> >>
> >> On Sun, 16 Mar 2025 17:26:55 +0000
> >> Paul Richard Thomas <paul.richard.tho...@gmail.com> wrote:
> >>
> >> > Hi All,
> >> >
> >> > This version of the REDUCE intrinsic patch has evolved somewhat since
> the
> >> > posting on 2nd March. The most important changes are to the wrapper
> >> > function and the addition of two testsuite entries.
> >> >
> >> > The wrapper function now effects:
> >> > subroutine wrapper (a, b, c)
> >> > type_of_ARRAY, intent(inout) :: a, c
> >> > type_of_ARRAY, intent(inout), optional :: b
> >> > if (present (b)) then
> >> > c = OPERATION (a,b )
> >> > else
> >> > c = a
> >> > end if
> >> > end subroutine
> >> >
> >> > The reason for wrapping OPERATION in a subroutine is to allow pointer
> >> > arithmetic to be used throughout in the library function. The only
> thing
> >> > that needs to be known about the type and kind of ARRAY is the element
> >> > size. The second branch in the wrapper allows deep copies to be done
> in the
> >> > library function, such that derived types with allocatable components
> do
> >> > not leak memory. This is needed at the final step of the algorithm to
> copy
> >> > the result from each iteration to the result and then nullify it.
> >> >
> >> > This is undoubtedly a bit heavy going for intrinsic types and so, one
> day
> >> > soon I will possibly do a bit of M4ery. That said, the present version
> >> > works for all types of ARRAY and I worry a bit about how much this
> >> > intrinsic will be used. Thoughts?
> >> >
> >> > A slight niggle is the linker error that comes up if compiled without
> any
> >> > optimization:
> >> > /usr/bin/ld: warning: /tmp/cc9cx9Rw.o: requires executable stack
> (because
> >> > the .note.GNU-stack section is executable)
> >> > I think that this is unlikely to present a security issue, however,
> since
> >> > it disappears at -O1, I went through each of the options triggered by
> -O1
> >> > but couldn't make it go away. Does anybody know why this is?
> >> >
> >> > Regtests OK with FC41/x86_64 - OK for mainline?
> >> >
> >> > Regards
> >> >
> >> > Paul
> >>
> >>
> >> --
> >> Andre Vehreschild * Email: vehre ad gmx dot de
>
_______________________________________________
linaro-toolchain mailing list -- linaro-toolchain@lists.linaro.org
To unsubscribe send an email to linaro-toolchain-le...@lists.linaro.org
