[patch, fortran] Implement maxloc and minloc for character

[patch, fortran] Implement maxloc and minloc for character

[Thomas Koenig]

[-- Attachment #1: Type: text/plain, Size: 4159 bytes --]

Hello world,

the attached patch implements maxloc and minloc, a missing feature / bug
(now that we are shooting for f2003 compliance). I decided to do
everything on the library side, since I am more familiar with that
territory. I also suspect that any performance gain from inlining will
be less pronounced than with intrinsic types.

There is one question regarding the ABI. Apparently, the string length
is passed as an int even on a 64-bit system. I verified that this
is indeed the case by doing the actual work on a
powerpc64-unknown-linux-gnu box (gcc110 on the gcc compile farm),
which is big-endian. If we were actually passing an eight-byte
quantity, and only getting the upper bytes, we would crash & burn.

Now, I _thought_ we were passing string lengths as size_t now (Janne?),
but maybe something was missing in that change.

So, this works, and passes regression testing. OK for trunk?
If so, I would tackle maxval next, in a similar fashion.
If anybody has another resolution for the size_t vs. int issue - the
nice thing about m4 is that it is fairly easy to make that change.

Regards

	Thomas


2017-11-19  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/36313
         * Makefile.am: Add i_maxloc0s_c, i_maxloc1s_c, i_maxloc2s_c,
         i_minloc0s_c, i_minloc1s_c and i_minloc2s_c.
         * Makefile.in: Regenerated.
         * generated/maxloc0_16_s1.c: New file.
         * generated/maxloc0_16_s4.c: New file.
         * generated/maxloc0_4_s1.c: New file.
         * generated/maxloc0_4_s4.c: New file.
         * generated/maxloc0_8_s1.c: New file.
         * generated/maxloc0_8_s4.c: New file.
         * generated/maxloc1_16_s1.c: New file.
         * generated/maxloc1_16_s4.c: New file.
         * generated/maxloc1_4_s1.c: New file.
         * generated/maxloc1_4_s4.c: New file.
         * generated/maxloc1_8_s1.c: New file.
         * generated/maxloc1_8_s4.c: New file.
         * generated/maxloc2_16_s1.c: New file.
         * generated/maxloc2_16_s4.c: New file.
         * generated/maxloc2_4_s1.c: New file.
         * generated/maxloc2_4_s4.c: New file.
         * generated/maxloc2_8_s1.c: New file.
         * generated/maxloc2_8_s4.c: New file.
         * generated/minloc0_16_s1.c: New file.
         * generated/minloc0_16_s4.c: New file.
         * generated/minloc0_4_s1.c: New file.
         * generated/minloc0_4_s4.c: New file.
         * generated/minloc0_8_s1.c: New file.
         * generated/minloc0_8_s4.c: New file.
         * generated/minloc1_16_s1.c: New file.
         * generated/minloc1_16_s4.c: New file.
         * generated/minloc1_4_s1.c: New file.
         * generated/minloc1_4_s4.c: New file.
         * generated/minloc1_8_s1.c: New file.
         * generated/minloc1_8_s4.c: New file.
         * generated/minloc2_16_s1.c: New file.
         * generated/minloc2_16_s4.c: New file.
         * generated/minloc2_4_s1.c: New file.
         * generated/minloc2_4_s4.c: New file.
         * generated/minloc2_8_s1.c: New file.
         * generated/minloc2_8_s4.c: New file.
         * m4/iforeach-s.m4: New file.
         * m4/ifunction-s.m4: New file.
         * m4/maxloc0s.m4: New file.
         * m4/maxloc1s.m4: New file.
         * m4/maxloc2s.m4: New file.
         * m4/minloc0s.m4: New file.
         * m4/minloc1s.m4: New file.
         * m4/minloc2s.m4: New file.
         * gfortran.map: Add new functions.
         * libgfortran.h: Add gfc_array_s1 and gfc_array_s4.

2017-11-19  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/36313
         * check.c (int_or_real_or_char_check_f2003): New function.
         * iresolve.c (gfc_resolve_maxloc): Add number "2" for
         character arguments and rank-zero return value.
         (gfc_resolve_minloc): Likewise.
         * trans-intrinsic.c (gfc_conv_intrinsic_minmaxloc): Handle case of
         character arguments and rank-zero return value by removing
         unneeded arguments and calling the library function.

2017-11-19  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/36313
         * gfortran.dg/maxloc_string_1.f90: New test.
         * gfortran.dg/minloc_string_1.f90: New test.

[-- Attachment #2: p7.diff.gz --]
[-- Type: application/gzip, Size: 25461 bytes --]

[-- Attachment #3: maxloc_string_1.f90 --]
[-- Type: text/x-fortran, Size: 2142 bytes --]

! { dg-do run }
! Test maxloc for strings for different code paths

program main
  implicit none
  integer, parameter :: n=4
  character(len=4), dimension(n,n) :: c
  integer, dimension(n,n) :: a
  integer, dimension(2) :: res1, res2
  real, dimension(n,n) :: r
  logical, dimension(n,n) :: amask
  logical(kind=8) :: smask
  integer :: i,j
  integer, dimension(n) :: q1, q2
  character(len=4,kind=4), dimension(n,n) :: c4
  character(len=4), dimension(n*n) :: e
  integer, dimension(n*n) :: f
  logical, dimension(n*n) :: cmask

  call random_number (r)
  a = int(r*100)
  do j=1,n
     do i=1,n
        write (unit=c(i,j),fmt='(I4.4)') a(i,j)
        write (unit=c4(i,j),fmt='(I4.4)') a(i,j)
     end do
  end do
  res1 = maxloc(c)
  res2 = maxloc(a)

  if (any(res1 /= res2)) call abort
  res1 = maxloc(c4)
  if (any(res1 /= res2)) call abort

  amask = a < 50
  res1 = maxloc(c,mask=amask)
  res2 = maxloc(a,mask=amask)

 if (any(res1 /= res2)) call abort

 amask = .false.
 res1 = maxloc(c,mask=amask)
 if (any(res1 /= 0)) call abort

 amask(2,3) = .true.
 res1 = maxloc(c,mask=amask)
 if (any(res1 /= [2,3])) call abort

 res1 = maxloc(c,mask=.false.)
 if (any(res1 /= 0)) call abort

 res2 = maxloc(a)
 res1 = maxloc(c,mask=.true.)
 if (any(res1 /= res2)) call abort

 q1 = maxloc(c, dim=1)
 q2 = maxloc(a, dim=1)
 if (any(q1 /= q2)) call abort

 q1 = maxloc(c, dim=2)
 q2 = maxloc(a, dim=2)
 if (any(q1 /= q2)) call abort

 q1 = maxloc(c, dim=1, mask=amask)
 q2 = maxloc(a, dim=1, mask=amask)
 if (any(q1 /= q2)) call abort

 q1 = maxloc(c, dim=2, mask=amask)
 q2 = maxloc(a, dim=2, mask=amask)
 if (any(q1 /= q2)) call abort

  amask = a < 50

 q1 = maxloc(c, dim=1, mask=amask)
 q2 = maxloc(a, dim=1, mask=amask)
 if (any(q1 /= q2)) call abort

 q1 = maxloc(c, dim=2, mask=amask)
 q2 = maxloc(a, dim=2, mask=amask)
 if (any(q1 /= q2)) call abort

 e = reshape(c, shape(e))
 f = reshape(a, shape(f))
 if (maxloc(e,dim=1) /= maxloc(f,dim=1)) call abort

 cmask = .false.
 if (maxloc(e,dim=1,mask=cmask) /= 0) call abort

 cmask = f > 50
 if ( maxloc(e, dim=1, mask=cmask) /= maxloc (f, dim=1, mask=cmask)) call abort
end program main

[-- Attachment #4: minloc_string_1.f90 --]
[-- Type: text/x-fortran, Size: 2142 bytes --]

! { dg-do run }
! Test minloc for strings for different code paths

program main
  implicit none
  integer, parameter :: n=4
  character(len=4), dimension(n,n) :: c
  integer, dimension(n,n) :: a
  integer, dimension(2) :: res1, res2
  real, dimension(n,n) :: r
  logical, dimension(n,n) :: amask
  logical(kind=8) :: smask
  integer :: i,j
  integer, dimension(n) :: q1, q2
  character(len=4,kind=4), dimension(n,n) :: c4
  character(len=4), dimension(n*n) :: e
  integer, dimension(n*n) :: f
  logical, dimension(n*n) :: cmask

  call random_number (r)
  a = int(r*100)
  do j=1,n
     do i=1,n
        write (unit=c(i,j),fmt='(I4.4)') a(i,j)
        write (unit=c4(i,j),fmt='(I4.4)') a(i,j)
     end do
  end do
  res1 = minloc(c)
  res2 = minloc(a)

  if (any(res1 /= res2)) call abort
  res1 = minloc(c4)
  if (any(res1 /= res2)) call abort

  amask = a < 50
  res1 = minloc(c,mask=amask)
  res2 = minloc(a,mask=amask)

 if (any(res1 /= res2)) call abort

 amask = .false.
 res1 = minloc(c,mask=amask)
 if (any(res1 /= 0)) call abort

 amask(2,3) = .true.
 res1 = minloc(c,mask=amask)
 if (any(res1 /= [2,3])) call abort

 res1 = minloc(c,mask=.false.)
 if (any(res1 /= 0)) call abort

 res2 = minloc(a)
 res1 = minloc(c,mask=.true.)
 if (any(res1 /= res2)) call abort

 q1 = minloc(c, dim=1)
 q2 = minloc(a, dim=1)
 if (any(q1 /= q2)) call abort

 q1 = minloc(c, dim=2)
 q2 = minloc(a, dim=2)
 if (any(q1 /= q2)) call abort

 q1 = minloc(c, dim=1, mask=amask)
 q2 = minloc(a, dim=1, mask=amask)
 if (any(q1 /= q2)) call abort

 q1 = minloc(c, dim=2, mask=amask)
 q2 = minloc(a, dim=2, mask=amask)
 if (any(q1 /= q2)) call abort

  amask = a < 50

 q1 = minloc(c, dim=1, mask=amask)
 q2 = minloc(a, dim=1, mask=amask)
 if (any(q1 /= q2)) call abort

 q1 = minloc(c, dim=2, mask=amask)
 q2 = minloc(a, dim=2, mask=amask)
 if (any(q1 /= q2)) call abort

 e = reshape(c, shape(e))
 f = reshape(a, shape(f))
 if (minloc(e,dim=1) /= minloc(f,dim=1)) call abort

 cmask = .false.
 if (minloc(e,dim=1,mask=cmask) /= 0) call abort

 cmask = f > 50
 if ( minloc(e, dim=1, mask=cmask) /= minloc (f, dim=1, mask=cmask)) call abort
end program main

Re: [patch, fortran] Implement maxloc and minloc for character

[Janne Blomqvist]

On Sun, Nov 19, 2017 at 11:11 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
> There is one question regarding the ABI. Apparently, the string length
> is passed as an int even on a 64-bit system. I verified that this
> is indeed the case by doing the actual work on a
> powerpc64-unknown-linux-gnu box (gcc110 on the gcc compile farm),
> which is big-endian. If we were actually passing an eight-byte
> quantity, and only getting the upper bytes, we would crash & burn.
>
> Now, I _thought_ we were passing string lengths as size_t now (Janne?),
> but maybe something was missing in that change.

Unfortunately I had to revert the charlen->size_t patch since it
caused regressions on aix/power (presumably due to endianness issues).
I did fix a potential bug there, but I never got any response to my
request to get an account on the gcc compile farm to test it, and gcc
7 stage3 was closing so I ran out of time.

There's apparently some other process for getting compile farm
accounts nowadays, and we have broken the ABI again for gcc 8, so
maybe I should dust off the patch and try again. Or what do you think?



-- 
Janne Blomqvist

Re: [patch, fortran] Implement maxloc and minloc for character

[Thomas Koenig]

Am 20.11.2017 um 09:30 schrieb Janne Blomqvist:
> On Sun, Nov 19, 2017 at 11:11 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
>> There is one question regarding the ABI. Apparently, the string length
>> is passed as an int even on a 64-bit system. I verified that this
>> is indeed the case by doing the actual work on a
>> powerpc64-unknown-linux-gnu box (gcc110 on the gcc compile farm),
>> which is big-endian. If we were actually passing an eight-byte
>> quantity, and only getting the upper bytes, we would crash & burn.
>>
>> Now, I _thought_ we were passing string lengths as size_t now (Janne?),
>> but maybe something was missing in that change.
> 
> Unfortunately I had to revert the charlen->size_t patch since it
> caused regressions on aix/power (presumably due to endianness issues).

Ah, that explains it. I had forgotten the reversion part.

> There's apparently some other process for getting compile farm
> accounts nowadays, and we have broken the ABI again for gcc 8, so
> maybe I should dust off the patch and try again. Or what do you think?

You can apply at https://cfarm.tetaneutral.net/ . These machines are
indeed quite nice to work on, especially because of the different
architectures (and because there are some very powerful machines
there).

So, any other comments about my patch? OK for trunk?

Regards

	Thomas

Re: [patch, fortran] Implement maxloc and minloc for character

[Janne Blomqvist]

On Mon, Nov 20, 2017 at 8:29 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Am 20.11.2017 um 09:30 schrieb Janne Blomqvist:
>>
>> On Sun, Nov 19, 2017 at 11:11 PM, Thomas Koenig <tkoenig@netcologne.de>
>> wrote:
>>>
>>> There is one question regarding the ABI. Apparently, the string length
>>> is passed as an int even on a 64-bit system. I verified that this
>>> is indeed the case by doing the actual work on a
>>> powerpc64-unknown-linux-gnu box (gcc110 on the gcc compile farm),
>>> which is big-endian. If we were actually passing an eight-byte
>>> quantity, and only getting the upper bytes, we would crash & burn.
>>>
>>> Now, I _thought_ we were passing string lengths as size_t now (Janne?),
>>> but maybe something was missing in that change.
>>
>>
>> Unfortunately I had to revert the charlen->size_t patch since it
>> caused regressions on aix/power (presumably due to endianness issues).
>
>
> Ah, that explains it. I had forgotten the reversion part.
>
>> There's apparently some other process for getting compile farm
>> accounts nowadays, and we have broken the ABI again for gcc 8, so
>> maybe I should dust off the patch and try again. Or what do you think?
>
>
> You can apply at https://cfarm.tetaneutral.net/ . These machines are
> indeed quite nice to work on, especially because of the different
> architectures (and because there are some very powerful machines
> there).
>
> So, any other comments about my patch? OK for trunk?

- In many cases the copyright notice has "This file is part of the GNU
Fortran 95 runtime library (libgfortran)." It's a while since we've
called ourselves "GNU Fortran 95", so just remove the "95".

- It seems in the library you're using int for string lengths? Please
use gfc_charlen_type instead (in the frontend, gfc_charlen_type_node).
(Most of the charlen->size_t patch is fixing up places where we're
accidentally using int instead of gfc_charlen_type..).

- Why are you using GFC_INTEGER_1 / GFC_INTEGER_4 to loop over the
arrays rather than char/gfc_char4_t? Not sure if it makes any
difference in practice, but it sure seems confusing..

- Not really related to your patch, but memcmp_char4 sure looks
redundant. Isn't it the same as memcmp(a, b, size*4), in which case we
could use optimized memcmp implementations?



-- 
Janne Blomqvist

Re: [patch, fortran] Implement maxloc and minloc for character

[Thomas Koenig]

[-- Attachment #1: Type: text/plain, Size: 2394 bytes --]

Hi Janne,


>> So, any other comments about my patch? OK for trunk?
> 
> - In many cases the copyright notice has "This file is part of the GNU
> Fortran 95 runtime library (libgfortran)." It's a while since we've
> called ourselves "GNU Fortran 95", so just remove the "95".

That's what I got for copying over an old version of maxloc
(when it still didn't have NAN handling) as a basis for my own
patch. This also meant that I had an old copyright notice. Fixed.


> - It seems in the library you're using int for string lengths? Please
> use gfc_charlen_type instead (in the frontend, gfc_charlen_type_node).
> (Most of the charlen->size_t patch is fixing up places where we're
> accidentally using int instead of gfc_charlen_type..).

Fixed.

> - Why are you using GFC_INTEGER_1 / GFC_INTEGER_4 to loop over the
> arrays rather than char/gfc_char4_t? Not sure if it makes any
> difference in practice, but it sure seems confusing..

The reason has to do with evil m4 magic. I used a macro
from iparm.m4, atype_code. Changing m4 code should mostly
be restricted to those cases where it is _really_ necessary
(people, say, not without justification, that m4 is a write-only
langauge).

> - Not really related to your patch, but memcmp_char4 sure looks
> redundant. Isn't it the same as memcmp(a, b, size*4), in which case we
> could use optimized memcmp implementations?

Big/little endian issues.

Consider the following short program:

#include <stdio.h>
#include <string.h>

char a[4] = { 1, 2, 3, 4};
char b[4] = { 4, 3, 2, 1};

int main()
{
   int i, j;
   memcpy (&i, a, sizeof(i));
   memcpy (&j, b, sizeof(j));
   printf("memcmp           : ");
   if (memcmp (&i,&j,sizeof(i)))
     printf("larger\n");
   else
     printf("smaller\n");

   printf("Direct comparison: ");
   if (i > j)
     printf("larger\n");
   else
     printf("smaller\n");

   return 0;
}

On my x86_64 system, this prints

memcmp           : larger 
 
 

Direct comparison: larger

On a big-endian system, this prints

memcmp           : larger
Direct comparison: smaller

However, I just learned about the __BYTE_ORDER__ macro.
We could use that (and in places where we currently have
a runtime check, for example in replacing the big_endian
global variable in libgfortran). But that is for another day.

So, attached is a new version of the patch. No update
on the ChangeLog. OK for trunk?

Regards

	Thomas

[-- Attachment #2: p8.diff.gz --]
[-- Type: application/gzip, Size: 24017 bytes --]

Re: [patch, fortran] Implement maxloc and minloc for character

[Janne Blomqvist]

On Tue, Nov 21, 2017 at 9:50 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Hi Janne,
>
>
>>> So, any other comments about my patch? OK for trunk?
>>
>>
>> - In many cases the copyright notice has "This file is part of the GNU
>> Fortran 95 runtime library (libgfortran)." It's a while since we've
>> called ourselves "GNU Fortran 95", so just remove the "95".
>
>
> That's what I got for copying over an old version of maxloc
> (when it still didn't have NAN handling) as a basis for my own
> patch. This also meant that I had an old copyright notice. Fixed.

Uh, it seems the patch you posted didn't actually fix this?

>> - It seems in the library you're using int for string lengths? Please
>> use gfc_charlen_type instead (in the frontend, gfc_charlen_type_node).
>> (Most of the charlen->size_t patch is fixing up places where we're
>> accidentally using int instead of gfc_charlen_type..).
>
>
> Fixed.

Not everywhere? At least

zgrep "int len" p8.diff.gz

turns up some cases?

>> - Why are you using GFC_INTEGER_1 / GFC_INTEGER_4 to loop over the
>> arrays rather than char/gfc_char4_t? Not sure if it makes any
>> difference in practice, but it sure seems confusing..
>
>
> The reason has to do with evil m4 magic. I used a macro
> from iparm.m4, atype_code. Changing m4 code should mostly
> be restricted to those cases where it is _really_ necessary
> (people, say, not without justification, that m4 is a write-only
> langauge).

Fair enough. :)

>> - Not really related to your patch, but memcmp_char4 sure looks
>> redundant. Isn't it the same as memcmp(a, b, size*4), in which case we
>> could use optimized memcmp implementations?
>
>
> Big/little endian issues.
>
> Consider the following short program:
>
> #include <stdio.h>
> #include <string.h>
>
> char a[4] = { 1, 2, 3, 4};
> char b[4] = { 4, 3, 2, 1};
>
> int main()
> {
>   int i, j;
>   memcpy (&i, a, sizeof(i));
>   memcpy (&j, b, sizeof(j));
>   printf("memcmp           : ");
>   if (memcmp (&i,&j,sizeof(i)))
>     printf("larger\n");
>   else
>     printf("smaller\n");
>
>   printf("Direct comparison: ");
>   if (i > j)
>     printf("larger\n");
>   else
>     printf("smaller\n");
>
>   return 0;
> }
>
> On my x86_64 system, this prints
>
> memcmp           : larger
>
>
> Direct comparison: larger
>
> On a big-endian system, this prints
>
> memcmp           : larger
> Direct comparison: smaller

Ooh, indeed.

> However, I just learned about the __BYTE_ORDER__ macro.
> We could use that (and in places where we currently have
> a runtime check, for example in replacing the big_endian
> global variable in libgfortran). But that is for another day.

Yup.

> So, attached is a new version of the patch. No update
> on the ChangeLog. OK for trunk?

Yup, just really fix the copyright and string length stuff first. Thanks!


-- 
Janne Blomqvist

Re: [patch, fortran] Implement maxloc and minloc for character

[Thomas Koenig]

Hi Janne,

>> So, attached is a new version of the patch. No update
>> on the ChangeLog. OK for trunk?
> Yup, just really fix the copyright and string length stuff first. Thanks!

Committed as rev 255070 with the fixes.

There are still some files which mention Fortran 95, that can be fixed
later.

Thanks a lot for the review!

Regards

	Thomas

Re: [patch, fortran] Implement maxloc and minloc for character

[Janne Blomqvist]

On Wed, Nov 22, 2017 at 8:10 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Hi Janne,
>
>>> So, attached is a new version of the patch. No update
>>> on the ChangeLog. OK for trunk?
>>
>> Yup, just really fix the copyright and string length stuff first. Thanks!
>
>
> Committed as rev 255070 with the fixes.
>
> There are still some files which mention Fortran 95, that can be fixed
> later.

That's ok, I wasn't expecting you to fix all such occurences, just the
new files you added.

However, to continue my nitpicking (sorry!), it seems that in many
cases compare_fcn still takes an integer length argument. Could you
make that gfc_charlen_type as well? Or maybe size_t, since the
argument is passed straight to memcmp{_char4} anyway? Please consider
such a patch pre-approved. Thanks!


-- 
Janne Blomqvist

Re: [patch, fortran] Implement maxloc and minloc for character

[Janne Blomqvist]

On Thu, Nov 23, 2017 at 2:56 PM, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> On Wed, Nov 22, 2017 at 8:10 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
>> Hi Janne,
>>
>>>> So, attached is a new version of the patch. No update
>>>> on the ChangeLog. OK for trunk?
>>>
>>> Yup, just really fix the copyright and string length stuff first. Thanks!
>>
>>
>> Committed as rev 255070 with the fixes.
>>
>> There are still some files which mention Fortran 95, that can be fixed
>> later.
>
> That's ok, I wasn't expecting you to fix all such occurences, just the
> new files you added.
>
> However, to continue my nitpicking (sorry!), it seems that in many
> cases compare_fcn still takes an integer length argument. Could you
> make that gfc_charlen_type as well? Or maybe size_t, since the
> argument is passed straight to memcmp{_char4} anyway? Please consider
> such a patch pre-approved. Thanks!

To continue, the prototypes are inconsistent too, e.g. m4/minloc2s.m4:

extern 'rtype_name` 'name`'rtype_qual`_'atype_code` ('atype` * const
restrict, int);
export_proto('name`'rtype_qual`_'atype_code`);

'rtype_name`
'name`'rtype_qual`_'atype_code` ('atype` * const restrict array,
gfc_charlen_type len)

See also lines 77/82, and in maxloc2s.m4 lines 42/46 and 144/149.

Thanks to James and Ramana on IRC, who are working on some target
where gfc_charlen_type != int and reported build failures.

-- 
Janne Blomqvist

Re: [patch, fortran] Implement maxloc and minloc for character

[Ramana Radhakrishnan]

On Thu, Nov 23, 2017 at 1:53 PM, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> On Thu, Nov 23, 2017 at 2:56 PM, Janne Blomqvist
> <blomqvist.janne@gmail.com> wrote:
>> On Wed, Nov 22, 2017 at 8:10 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
>>> Hi Janne,
>>>
>>>>> So, attached is a new version of the patch. No update
>>>>> on the ChangeLog. OK for trunk?
>>>>
>>>> Yup, just really fix the copyright and string length stuff first. Thanks!
>>>
>>>
>>> Committed as rev 255070 with the fixes.
>>>
>>> There are still some files which mention Fortran 95, that can be fixed
>>> later.
>>
>> That's ok, I wasn't expecting you to fix all such occurences, just the
>> new files you added.
>>
>> However, to continue my nitpicking (sorry!), it seems that in many
>> cases compare_fcn still takes an integer length argument. Could you
>> make that gfc_charlen_type as well? Or maybe size_t, since the
>> argument is passed straight to memcmp{_char4} anyway? Please consider
>> such a patch pre-approved. Thanks!
>
> To continue, the prototypes are inconsistent too, e.g. m4/minloc2s.m4:
>
> extern 'rtype_name` 'name`'rtype_qual`_'atype_code` ('atype` * const
> restrict, int);
> export_proto('name`'rtype_qual`_'atype_code`);
>
> 'rtype_name`
> 'name`'rtype_qual`_'atype_code` ('atype` * const restrict array,
> gfc_charlen_type len)
>
> See also lines 77/82, and in maxloc2s.m4 lines 42/46 and 144/149.
>
> Thanks to James and Ramana on IRC, who are working on some target
> where gfc_charlen_type != int and reported build failures.


I'm not sure why gfc_charlen_type != int on arm-none-eabi and
aarch64-none-elf which is where both James and I saw the issue.

The issue hasn't appeared on any of our cross-linux builds.

regards
Ramana

>
> --
> Janne Blomqvist

Re: [patch, fortran] Implement maxloc and minloc for character

[Janne Blomqvist]

On Thu, Nov 23, 2017 at 3:58 PM, Ramana Radhakrishnan
<ramana.gcc@googlemail.com> wrote:
> On Thu, Nov 23, 2017 at 1:53 PM, Janne Blomqvist
> <blomqvist.janne@gmail.com> wrote:
>> On Thu, Nov 23, 2017 at 2:56 PM, Janne Blomqvist
>> <blomqvist.janne@gmail.com> wrote:
>>> On Wed, Nov 22, 2017 at 8:10 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
>>>> Hi Janne,
>>>>
>>>>>> So, attached is a new version of the patch. No update
>>>>>> on the ChangeLog. OK for trunk?
>>>>>
>>>>> Yup, just really fix the copyright and string length stuff first. Thanks!
>>>>
>>>>
>>>> Committed as rev 255070 with the fixes.
>>>>
>>>> There are still some files which mention Fortran 95, that can be fixed
>>>> later.
>>>
>>> That's ok, I wasn't expecting you to fix all such occurences, just the
>>> new files you added.
>>>
>>> However, to continue my nitpicking (sorry!), it seems that in many
>>> cases compare_fcn still takes an integer length argument. Could you
>>> make that gfc_charlen_type as well? Or maybe size_t, since the
>>> argument is passed straight to memcmp{_char4} anyway? Please consider
>>> such a patch pre-approved. Thanks!
>>
>> To continue, the prototypes are inconsistent too, e.g. m4/minloc2s.m4:
>>
>> extern 'rtype_name` 'name`'rtype_qual`_'atype_code` ('atype` * const
>> restrict, int);
>> export_proto('name`'rtype_qual`_'atype_code`);
>>
>> 'rtype_name`
>> 'name`'rtype_qual`_'atype_code` ('atype` * const restrict array,
>> gfc_charlen_type len)
>>
>> See also lines 77/82, and in maxloc2s.m4 lines 42/46 and 144/149.
>>
>> Thanks to James and Ramana on IRC, who are working on some target
>> where gfc_charlen_type != int and reported build failures.
>
>
> I'm not sure why gfc_charlen_type != int on arm-none-eabi and
> aarch64-none-elf which is where both James and I saw the issue.
>
> The issue hasn't appeared on any of our cross-linux builds.

gfc_charlen_type is typedeffed to a GFC_INTEGER_4, which comes from
kinds.h generated during the library build time (see
libgfortran/mk-kinds-h.sh). At least on x86_64-pc-linux-gnu this is
then a int32_t which is a typedef for plain int. HTH.



-- 
Janne Blomqvist

Re: [patch, fortran] Implement maxloc and minloc for character

[Thomas Koenig]

Hi Janne,

> However, to continue my nitpicking (sorry!), it seems that in many
> cases compare_fcn still takes an integer length argument. Could you
> make that gfc_charlen_type as well? Or maybe size_t, since the
> argument is passed straight to memcmp{_char4} anyway? Please consider
> such a patch pre-approved. Thanks!

Committed as r255109.

I had missed out on the non-inlined maxval and maxloc versions...

The fun with max* and min* intrinsics is not yet over. Maxval and
Minval have yet to be implemented for character arguments, and then
there is the BACK argument to MAXLOC.

Maybe (while we are breaking compatibility) we should just add BACK
to the front end, reject it whith a "not yet implemented" message,
add the argument to the library and worry about implementation
later.

What do you think?

Regards

	Thomas

Re: [patch, fortran] Implement maxloc and minloc for character

[Janne Blomqvist]

On Thu, Nov 23, 2017 at 7:58 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Hi Janne,
>
>> However, to continue my nitpicking (sorry!), it seems that in many
>> cases compare_fcn still takes an integer length argument. Could you
>> make that gfc_charlen_type as well? Or maybe size_t, since the
>> argument is passed straight to memcmp{_char4} anyway? Please consider
>> such a patch pre-approved. Thanks!
>
>
> Committed as r255109.
>
> I had missed out on the non-inlined maxval and maxloc versions...

You still missed two places. Committed r255135 with the fixes.

> The fun with max* and min* intrinsics is not yet over. Maxval and
> Minval have yet to be implemented for character arguments, and then
> there is the BACK argument to MAXLOC.
>
> Maybe (while we are breaking compatibility) we should just add BACK
> to the front end, reject it whith a "not yet implemented" message,
> add the argument to the library and worry about implementation
> later.
>
> What do you think?

Yeah, makes sense I suppose.


-- 
Janne Blomqvist