| Message ID | trinity-8c340681-a917-4131-9801-344993efb907-1637351227189@3c-app-gmx-bap45 |
|---|---|
| State | New |
| Headers | show |
| Series | PR fortran/87851 - [9/10/11/12 Regression] Wrong return type for len_trim | expand |
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.
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
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,
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
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