On Wed, Mar 19, 2025 at 11:23 AM 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.
I also saw it on x86-64: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119540 > 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 -- H.J.
