[PATCH] PR fortran/87851 - [9/10/11/12 Regression] Wrong return type for len_trim

[PATCH] PR fortran/87851 - [9/10/11/12 Regression] Wrong return type for len_trim

[Harald Anlauf]

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

Dear Fortranners,

scalariziation of the elemental intrinsic LEN_TRIM was ICEing
when the optional KIND argument was present.

The cleanest solution is to use the infrastructure added by
Mikael's fix for PR97896.  In that case it is a 1-liner.  :-)

That fix is available on mainline and on 11-branch only, though.
My suggestion is to fix the current PR only for the same branches,
leaving the regression unfixed for older ones.

Regtested on x86_64-pc-linux-gnu.  OK for mainline and 11-branch?

Thanks,
Harald


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fortran-fix-scalarization-for-intrinsic-LEN_TRIM-wit.patch --]
[-- Type: text/x-patch, Size: 2479 bytes --]

From f700c43fef4a239af25dd30dc22930b1bb1dbe95 Mon Sep 17 00:00:00 2001
From: Harald Anlauf <anlauf@gmx.de>
Date: Fri, 19 Nov 2021 20:20:44 +0100
Subject: [PATCH] Fortran: fix scalarization for intrinsic LEN_TRIM with
 present KIND argument

gcc/fortran/ChangeLog:

	PR fortran/87851
	* trans-array.c (arg_evaluated_for_scalarization): Add LEN_TRIM to
	list of intrinsics for which an optional KIND argument needs to be
	removed before scalarization.

gcc/testsuite/ChangeLog:

	PR fortran/87851
	* gfortran.dg/len_trim.f90: New test.
---
 gcc/fortran/trans-array.c              |  1 +
 gcc/testsuite/gfortran.dg/len_trim.f90 | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 gcc/testsuite/gfortran.dg/len_trim.f90

diff --git a/gcc/fortran/trans-array.c b/gcc/fortran/trans-array.c
index 2090adf01e7..238b1b72385 100644
--- a/gcc/fortran/trans-array.c
+++ b/gcc/fortran/trans-array.c
@@ -11499,6 +11499,7 @@ arg_evaluated_for_scalarization (gfc_intrinsic_sym *function,
       switch (function->id)
 	{
 	  case GFC_ISYM_INDEX:
+	  case GFC_ISYM_LEN_TRIM:
 	    if (strcmp ("kind", gfc_dummy_arg_get_name (*dummy_arg)) == 0)
 	      return false;
 	  /* Fallthrough.  */
diff --git a/gcc/testsuite/gfortran.dg/len_trim.f90 b/gcc/testsuite/gfortran.dg/len_trim.f90
new file mode 100644
index 00000000000..63f803960d5
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/len_trim.f90
@@ -0,0 +1,26 @@
+! { dg-do compile }
+! { dg-options "-O -Wall -Wconversion-extra -fdump-tree-original" }
+! { dg-final { scan-tree-dump-not "_gfortran_stop_numeric" "original" } }
+! PR fortran/87851 - return type for len_trim
+
+program main
+  implicit none
+  character(3), parameter :: a(1) = 'aa'
+  character(3)            :: b    = "bb"
+  character(3)            :: c(1) = 'cc'
+  integer(4), parameter   :: l4(1) = len_trim (a, kind=4)
+  integer(8), parameter   :: l8(1) = len_trim (a, kind=8)
+  integer                 :: kk(1) = len_trim (a)
+  integer(4)              :: mm(1) = len_trim (a, kind=4)
+  integer(8)              :: nn(1) = len_trim (a, kind=8)
+  kk = len_trim (a)
+  mm = len_trim (a, kind=4)
+  nn = len_trim (a, kind=8)
+  kk = len_trim ([b])
+  mm = len_trim ([b],kind=4)
+  nn = len_trim ([b],kind=8)
+  kk = len_trim (c)
+  mm = len_trim (c, kind=4)
+  nn = len_trim (c, kind=8)
+  if (any (l4 /= 2_4) .or. any (l8 /= 2_8)) stop 1
+end program main
--
2.26.2

Re: [PATCH] PR fortran/87851 - [9/10/11/12 Regression] Wrong return type for len_trim

[Mikael Morin]

Le 19/11/2021 à 20:47, Harald Anlauf via Fortran a écrit :
> Dear Fortranners,
> 
> scalariziation of the elemental intrinsic LEN_TRIM was ICEing
> when the optional KIND argument was present.
> 
> The cleanest solution is to use the infrastructure added by
> Mikael's fix for PR97896.  In that case it is a 1-liner.  :-)
> 
> That fix is available on mainline and on 11-branch only, though.
> My suggestion is to fix the current PR only for the same branches,
> leaving the regression unfixed for older ones.
> 
> Regtested on x86_64-pc-linux-gnu.  OK for mainline and 11-branch?
> 
Your change itself is fine.
The PR was originally about a type mismatch between the gfortran library 
and the call generated by the front-end.
As the code generated contains a cast, I think it’s fine as well.
But please give Thomas (bug reporter) one more day to comment on this.
Then I think you can proceed.

Thanks.

Re: [PATCH] PR fortran/87851 - [9/10/11/12 Regression] Wrong return type for len_trim

[Harald Anlauf]

Am 21.11.21 um 12:46 schrieb Mikael Morin:
> Le 19/11/2021 à 20:47, Harald Anlauf via Fortran a écrit :
>> Dear Fortranners,
>>
>> scalariziation of the elemental intrinsic LEN_TRIM was ICEing
>> when the optional KIND argument was present.
>>
>> The cleanest solution is to use the infrastructure added by
>> Mikael's fix for PR97896.  In that case it is a 1-liner.  :-)
>>
>> That fix is available on mainline and on 11-branch only, though.
>> My suggestion is to fix the current PR only for the same branches,
>> leaving the regression unfixed for older ones.
>>
>> Regtested on x86_64-pc-linux-gnu.  OK for mainline and 11-branch?
>>
> Your change itself is fine.
> The PR was originally about a type mismatch between the gfortran library
> and the call generated by the front-end.

The supposed type mismatch was due to Janne's commit r8-5772:

commit f622221ab42c4ca550059add89ffda00ed2b3c03
Author: Janne Blomqvist <jb@gcc.gnu.org>
Date:   Fri Jan 5 21:01:12 2018 +0200

     PR 78534 Change character length from int to size_t

     In order to handle large character lengths on (L)LP64 targets, switch
     the GFortran character length from an int to a size_t.

     This is an ABI change, as procedures with character arguments take
     hidden arguments with the character length.
[...]

LEN_TRIM is of course of type default integer, which is handled fine
in gfc_resolve_len_trim, setting f->ts.kind, the same way as it is
done in gfc_resolve_index_func or elsewhere, and conversions are
properly taken care of as far as I can tell.

Things work(ed) fine for the "scalar" version of LEN_TRIM, even if the
optional KIND argument was present, before the above commit.  But not
the elemental version working on rank>=1 with KIND present.  That did
not change.  See PR87711.

Thinking again, the patch primarily addresses PR87711 (for 11/12) and
fixes some of the comments in PR87851.  We'd need to find out what
exactly is left from the latter.

The dump-tree of the testcase looks fine to me, even when compiling
with -fdefault-integer-8, and I do not see any conversion warnings.

> As the code generated contains a cast, I think it’s fine as well.

Well, LEN_TRIM is of default integer, unless KIND is given.

> But please give Thomas (bug reporter) one more day to comment on this.

Sure.

> Then I think you can proceed.
>
> Thanks.
>

Thanks,
Harald

Re: [PATCH] PR fortran/87851 - [9/10/11/12 Regression] Wrong return type for len_trim

[Harald Anlauf]

Am 21.11.21 um 12:46 schrieb Mikael Morin:
> Le 19/11/2021 à 20:47, Harald Anlauf via Fortran a écrit :
>> Dear Fortranners,
>>
>> scalariziation of the elemental intrinsic LEN_TRIM was ICEing
>> when the optional KIND argument was present.
>>
>> The cleanest solution is to use the infrastructure added by
>> Mikael's fix for PR97896.  In that case it is a 1-liner.  :-)
>>
>> That fix is available on mainline and on 11-branch only, though.
>> My suggestion is to fix the current PR only for the same branches,
>> leaving the regression unfixed for older ones.
>>
>> Regtested on x86_64-pc-linux-gnu.  OK for mainline and 11-branch?
>>
> Your change itself is fine.
> The PR was originally about a type mismatch between the gfortran library 
> and the call generated by the front-end.

The supposed type mismatch was due to Janne's commit r8-5772:

commit f622221ab42c4ca550059add89ffda00ed2b3c03
Author: Janne Blomqvist <jb@gcc.gnu.org>
Date:   Fri Jan 5 21:01:12 2018 +0200

     PR 78534 Change character length from int to size_t

     In order to handle large character lengths on (L)LP64 targets, switch
     the GFortran character length from an int to a size_t.

     This is an ABI change, as procedures with character arguments take
     hidden arguments with the character length.
[...]

LEN_TRIM is of course of type default integer, which is handled fine
in gfc_resolve_len_trim, setting f->ts.kind, the same way as it is
done in gfc_resolve_index_func or elsewhere, and conversions are
properly taken care of as far as I can tell.

Things work(ed) fine for the "scalar" version of LEN_TRIM, even if the
optional KIND argument was present, before the above commit.  But not
the elemental version working on rank>=1 with KIND present.  That did
not change.  See PR87711.

Thinking again, the patch primarily addresses PR87711 (for 11/12) and
fixes some of the comments in PR87851.  We'd need to find out what
exactly is left from the latter.

The dump-tree of the testcase looks fine to me, even when compiling
with -fdefault-integer-8, and I do not see any conversion warnings.

> As the code generated contains a cast, I think it’s fine as well.

Well, LEN_TRIM is of default integer, unless KIND is given.

> But please give Thomas (bug reporter) one more day to comment on this.

Sure.

> Then I think you can proceed.
> 
> Thanks.
> 

Thanks,
Harald

Re: [PATCH] PR fortran/87851 - [9/10/11/12 Regression] Wrong return type for len_trim

[Bernhard Reutner-Fischer]

On Mon, 22 Nov 2021 19:17:51 +0100
Harald Anlauf via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> Am 21.11.21 um 12:46 schrieb Mikael Morin:
> > Le 19/11/2021 à 20:47, Harald Anlauf via Fortran a écrit :  
> >> Dear Fortranners,
> >>
> >> scalariziation of the elemental intrinsic LEN_TRIM was ICEing
> >> when the optional KIND argument was present.
> >>
> >> The cleanest solution is to use the infrastructure added by
> >> Mikael's fix for PR97896.  In that case it is a 1-liner.  :-)

I'm just wondering loud if it would be more convenient to have a
unsigned hidden_arg:1 bit in let's say gfc_actual_arglist that denotes
if the argument should be const eval'ed and used before, and, most
importantly not passed to the library. We seem to have more than just
the index intrinsic's kind arg in that boat. And from what i read,
powerpc will eventuall want to select quite some kind-specific library
functions soon, depending on how this part is implemented..

Maybe add SPEC_HIDDEN_ARG / SPEC_LIBRARY_SELECTOR additional
gfc_param_spec_type if a separate bit is deemed inappropriate.

Such a hidden_arg/library_selector/non_library_call_arg flag is maybe
better than matching individual functions and strcmp the arg name.

cheers,

Re: [PATCH] PR fortran/87851 - [9/10/11/12 Regression] Wrong return type for len_trim

[Harald Anlauf]

Am 22.11.21 um 21:30 schrieb Bernhard Reutner-Fischer via Fortran:
> On Mon, 22 Nov 2021 19:17:51 +0100
> Harald Anlauf via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
>> Am 21.11.21 um 12:46 schrieb Mikael Morin:
>>> Le 19/11/2021 à 20:47, Harald Anlauf via Fortran a écrit :
>>>> Dear Fortranners,
>>>>
>>>> scalariziation of the elemental intrinsic LEN_TRIM was ICEing
>>>> when the optional KIND argument was present.
>>>>
>>>> The cleanest solution is to use the infrastructure added by
>>>> Mikael's fix for PR97896.  In that case it is a 1-liner.  :-)
> 
> I'm just wondering loud if it would be more convenient to have a
> unsigned hidden_arg:1 bit in let's say gfc_actual_arglist that denotes
> if the argument should be const eval'ed and used before, and, most
> importantly not passed to the library. We seem to have more than just
> the index intrinsic's kind arg in that boat. And from what i read,
> powerpc will eventuall want to select quite some kind-specific library
> functions soon, depending on how this part is implemented..

Well, that does not make sense for KIND, which has to be constant.
We even have an appropriate check for this in check.c(kind_check).

And KIND may even select a special library function, which means
that KIND cannot be an ordinary function argument.

> Maybe add SPEC_HIDDEN_ARG / SPEC_LIBRARY_SELECTOR additional
> gfc_param_spec_type if a separate bit is deemed inappropriate.
> 
> Such a hidden_arg/library_selector/non_library_call_arg flag is maybe
> better than matching individual functions and strcmp the arg name.
> 
> cheers,
> 

Re: [PATCH] PR fortran/87851 - [9/10/11/12 Regression] Wrong return type for len_trim

[Mikael Morin]

Le 22/11/2021 à 21:30, Bernhard Reutner-Fischer a écrit :
> 
> I'm just wondering loud if it would be more convenient to have a
> unsigned hidden_arg:1 bit in let's say gfc_actual_arglist that denotes
> if the argument should be const eval'ed and used before, and, most
> importantly not passed to the library. We seem to have more than just
> the index intrinsic's kind arg in that boat. And from what i read,
> powerpc will eventuall want to select quite some kind-specific library
> functions soon, depending on how this part is implemented..
> 
> Maybe add SPEC_HIDDEN_ARG / SPEC_LIBRARY_SELECTOR additional
> gfc_param_spec_type if a separate bit is deemed inappropriate.
> 
> Such a hidden_arg/library_selector/non_library_call_arg flag is maybe
> better than matching individual functions and strcmp the arg name.
> 
Hello,

I prefer not to go that way if possible:
  - because additional flags have a maintenance cost; it’s an additional 
complexity in the core structures, which impacts the whole compiler; 
it’s additional code to set them up, and maintainers have to understand 
what they are for, where they matter and where they don’t.
  - because the flag would have to be set at some point somewhere, which 
would probably be by matching individual functions and argument names; 
so the result would be the same.

You seem to be mostly concerned by the performance penalty, but I think 
4 characters string comparisons at compile time don’t matter in 
practice, as long as there aren’t millions of them.

Regarding the powerpc floating point representation and kind problem, 
let’s see what we need when we really need it. ;-)

Mikael