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
