[Patch, fortran] PR85836: Implement the F2018 reduce intrinsic

[H.J. Lu]

On Wed, Mar 19, 2025 at 11:23 AM Paul Richard Thomas
<paul.richard.thomas@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 <vehre@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.thomas@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.