Hi Christophe,
I have posted what I think is a fix for this problem with comment 14 of
PR119460. I would be grateful if you or one of your colleagues would give
it a spin at your earliest convenience.
It will be submitted before Monday morning.
Best regards
Paul
On Thu, 3 Apr 2025 at 17:32, Christophe Lyon <christophe.l...@linaro.org>
wrote:
> On Thu, 3 Apr 2025 at 17:55, Paul Richard Thomas
> <paul.richard.tho...@gmail.com> wrote:
> >
> > 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.
> >
> Hi!
>
> Yes with your patch the test passes on arm-unknown-linux-gnueabihf.
>
> > Je vous en remercie d'avance.
> You're welcome!
>
> Thanks,
>
> Christophe
>
> > 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
