Re: [Patch, fortran] PR96087 - [12/13/14/15 Regression] ICE in gfc_get_symbol_decl, at fortran/trans-decl.c:1575
Hi Paul, the patch looks reasonable to me. Ok for mainline. Just a side-thought: Could it be possible, that the for-loop in trans-decl does not find the result? Would an assert after the loop at least give a hint, where something went wrong? That's just from reading the code, so if you think that can not happen, feel free to commit w/o the assert. Regards, Andre On Wed, 22 Jan 2025 14:03:15 + Paul Richard Thomas wrote: > Hi All, > > This patch fixes a double ICE arising from confusion between the dummy > symbol arising from a module function/subroutine interface and the module > procedure itself. In both cases, use of the name is unambiguous, as > explained in the ChangeLog. The testcase contains both the original and the > variant in comment 1. > > Regtests OK with FC41/x86_64 - OK for mainline and later backporting? > > Paul > > Fortran: Regression- fix ICE at fortran/trans-decl.c:1575 [PR96087] > > 2025-01-22 Paul Thomas > > gcc/fortran > PR fortran/96087 > * trans-decl.cc (gfc_get_symbol_decl): If a dummy is missing a > backend decl, it is likely that it has come from a module proc > interface. Look for the formal symbol by name in the containing > proc and use its backend decl. > * trans-expr.cc (gfc_apply_interface_mapping_to_expr): For the > same reason, match the name, rather than the symbol address to > perform the mapping. > > gcc/testsuite/ > PR fortran/96087 > * gfortran.dg/pr96087.f90: New test. -- Andre Vehreschild * Email: vehre ad gmx dot de
[Patch, fortran] PR96087 - [12/13/14/15 Regression] ICE in gfc_get_symbol_decl, at fortran/trans-decl.c:1575
Hi All,
This patch fixes a double ICE arising from confusion between the dummy
symbol arising from a module function/subroutine interface and the module
procedure itself. In both cases, use of the name is unambiguous, as
explained in the ChangeLog. The testcase contains both the original and the
variant in comment 1.
Regtests OK with FC41/x86_64 - OK for mainline and later backporting?
Paul
Fortran: Regression- fix ICE at fortran/trans-decl.c:1575 [PR96087]
2025-01-22 Paul Thomas
gcc/fortran
PR fortran/96087
* trans-decl.cc (gfc_get_symbol_decl): If a dummy is missing a
backend decl, it is likely that it has come from a module proc
interface. Look for the formal symbol by name in the containing
proc and use its backend decl.
* trans-expr.cc (gfc_apply_interface_mapping_to_expr): For the
same reason, match the name, rather than the symbol address to
perform the mapping.
gcc/testsuite/
PR fortran/96087
* gfortran.dg/pr96087.f90: New test.
diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index 4ae22a5584d..97bb0a41858 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -1722,6 +1722,21 @@ gfc_get_symbol_decl (gfc_symbol * sym)
sym->backend_decl = DECL_CHAIN (sym->backend_decl);
}
+ /* Automatic array indices in module procedures need the backend_decl
+ to be extracted from the procedure formal arglist. */
+ if (sym->attr.dummy && !sym->backend_decl)
+ {
+ gfc_formal_arglist *f;
+ for (f = sym->ns->proc_name->formal; f; f = f->next)
+ {
+ gfc_symbol *fsym = f->sym;
+ if (strcmp (sym->name, fsym->name))
+ continue;
+ sym->backend_decl = fsym->backend_decl;
+ break;
+ }
+ }
+
/* Dummy variables should already have been created. */
gcc_assert (sym->backend_decl);
diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index bef49d32a58..1bf46c616bb 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -5099,7 +5099,7 @@ gfc_apply_interface_mapping_to_expr (gfc_interface_mapping * mapping,
/* TODO Find out why the condition on expr->symtree had to be moved into
the loop rather than being outside it, as originally. */
for (sym = mapping->syms; sym; sym = sym->next)
-if (expr->symtree && sym->old == expr->symtree->n.sym)
+if (expr->symtree && !strcmp (sym->old->name, expr->symtree->n.sym->name))
{
if (sym->new_sym->n.sym->backend_decl)
expr->symtree = sym->new_sym;
diff --git a/gcc/testsuite/gfortran.dg/pr96087.f90 b/gcc/testsuite/gfortran.dg/pr96087.f90
new file mode 100644
index 000..6c75d4f0cf2
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr96087.f90
@@ -0,0 +1,46 @@
+! { dg-do run }
+
+module m
+ interface
+ module function f(a, n, b) result(z)
+ integer, intent(in) :: n
+ real :: z(n + 1)
+ real :: a, b
+ end
+ end interface
+contains
+ module procedure f
+ integer :: i
+ do i = 1, size(z)
+z(i) = real(i)
+ end do
+ end procedure
+end
+
+! Comment 1
+module n
+ interface
+ module subroutine g(n, z)
+ integer, intent(in) :: n
+ real :: z(n)
+ end
+ end interface
+contains
+ module procedure g
+ z = 1
+ if (int (sum (z)) /= n) stop 1
+ end procedure
+end
+
+ use m
+ use n
+ real, allocatable :: r(:)
+ integer :: i = 2
+ r = f (1.0, i+1, 2.0)
+ if (any (r .ne. [(real(i), i = 1,4)])) stop 2
+ if (any (f (3.0, 1, 4.0) .ne. [(real(i), i = 1,2)])) stop 3
+
+ r = [(real (i), i = 10,20)]
+ call g (5, r)
+ if (int (sum (r)) /= (sum ([(i, i = 15,20)]) + 5)) stop 4
+end
[PATCH] Fortran: do not evaluate arguments of MAXVAL/MINVAL too often [PR118613]
Dear all,
while looking at details of a related but slightly different PR, I found
that we did evaluate the arguments to MINLOC/MAXLOC too often in the
inlined version.
The attached patch creates temporaries for array elements where needed,
and ensures that each array element is only touched once. This required
a minor adjustment for the rank-1 algorithm, which is documented.
Regtested on x86_64-pc-linux-gnu. OK for mainline?
Thanks,
Harald
From d2a52256b3e4817e16a5d222c2fecd7bc66e5613 Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Wed, 22 Jan 2025 22:44:39 +0100
Subject: [PATCH] Fortran: do not evaluate arguments of MAXVAL/MINVAL too often
[PR118613]
PR fortran/118613
gcc/fortran/ChangeLog:
* trans-intrinsic.cc (gfc_conv_intrinsic_minmaxval): Adjust algorithm
for inlined version of MINLOC and MAXLOC so that arguments are only
evaluted once, and create temporaries where necessary. Document
change of algorithm.
gcc/testsuite/ChangeLog:
* gfortran.dg/maxval_arg_eval_count.f90: New test.
---
gcc/fortran/trans-intrinsic.cc| 37 +++-
.../gfortran.dg/maxval_arg_eval_count.f90 | 88 +++
2 files changed, 121 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/maxval_arg_eval_count.f90
diff --git a/gcc/fortran/trans-intrinsic.cc b/gcc/fortran/trans-intrinsic.cc
index afbec5b2752..51237d0d3be 100644
--- a/gcc/fortran/trans-intrinsic.cc
+++ b/gcc/fortran/trans-intrinsic.cc
@@ -6409,8 +6409,16 @@ gfc_conv_intrinsic_findloc (gfc_se *se, gfc_expr *expr)
nonempty = false;
S = from;
while (S <= to) {
- if (mask[S]) { nonempty = true; if (a[S] <= limit) goto lab; }
- S++;
+ if (mask[S]) {
+ nonempty = true;
+ if (a[S] <= limit) {
+ limit = a[S];
+ S++;
+ goto lab;
+ }
+ else
+ S++;
+ }
}
limit = nonempty ? NaN : huge (limit);
lab:
@@ -6419,7 +6427,15 @@ gfc_conv_intrinsic_findloc (gfc_se *se, gfc_expr *expr)
at runtime whether array is nonempty or not, rank 1:
limit = Infinity;
S = from;
- while (S <= to) { if (a[S] <= limit) goto lab; S++; }
+ while (S <= to) {
+ if (a[S] <= limit) {
+ limit = a[S];
+ S++;
+ goto lab;
+ }
+ else
+ S++;
+ }
limit = (from <= to) ? NaN : huge (limit);
lab:
while (S <= to) { limit = min (a[S], limit); S++; }
@@ -6710,6 +6726,7 @@ gfc_conv_intrinsic_minmaxval (gfc_se * se, gfc_expr * expr, enum tree_code op)
gfc_copy_loopinfo_to_se (&arrayse, &loop);
arrayse.ss = arrayss;
gfc_conv_expr_val (&arrayse, arrayexpr);
+ arrayse.expr = gfc_evaluate_now (arrayse.expr, &arrayse.pre);
gfc_add_block_to_block (&block, &arrayse.pre);
gfc_init_block (&block2);
@@ -6722,7 +6739,18 @@ gfc_conv_intrinsic_minmaxval (gfc_se * se, gfc_expr * expr, enum tree_code op)
tmp = fold_build2_loc (input_location, op == GT_EXPR ? GE_EXPR : LE_EXPR,
logical_type_node, arrayse.expr, limit);
if (lab)
- ifbody = build1_v (GOTO_EXPR, lab);
+ {
+ stmtblock_t ifblock;
+ tree inc_loop;
+ inc_loop = fold_build2_loc (input_location, PLUS_EXPR,
+ TREE_TYPE (loop.loopvar[0]),
+ loop.loopvar[0], gfc_index_one_node);
+ gfc_init_block (&ifblock);
+ gfc_add_modify (&ifblock, limit, arrayse.expr);
+ gfc_add_modify (&ifblock, loop.loopvar[0], inc_loop);
+ gfc_add_expr_to_block (&ifblock, build1_v (GOTO_EXPR, lab));
+ ifbody = gfc_finish_block (&ifblock);
+ }
else
{
stmtblock_t ifblock;
@@ -6816,6 +6844,7 @@ gfc_conv_intrinsic_minmaxval (gfc_se * se, gfc_expr * expr, enum tree_code op)
gfc_copy_loopinfo_to_se (&arrayse, &loop);
arrayse.ss = arrayss;
gfc_conv_expr_val (&arrayse, arrayexpr);
+ arrayse.expr = gfc_evaluate_now (arrayse.expr, &arrayse.pre);
gfc_add_block_to_block (&block, &arrayse.pre);
/* MIN_EXPR/MAX_EXPR has unspecified behavior with NaNs or
diff --git a/gcc/testsuite/gfortran.dg/maxval_arg_eval_count.f90 b/gcc/testsuite/gfortran.dg/maxval_arg_eval_count.f90
new file mode 100644
index 000..1c1a04819a0
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/maxval_arg_eval_count.f90
@@ -0,0 +1,88 @@
+! { dg-do run }
+!
+! PR fortran/118613 - check argument evaluation count of MAXVAL
+
+program p
+ implicit none
+ integer, parameter :: k = 2
+ integer :: n
+ integer :: i1(k*k), i2(k,k), mm
+ real:: a1(k*k), a2(k,k), mx
+ complex :: c1(k*k), c2(k,k)
+ logical :: m1(k*k), m2(k,k)
+
+ ! prepare mask for masked variants
+ m1 = .true.
+ m2 = .true.
+ i1 = 0
+ i2 = 0
+ a1 = 0.
+ a2 = 0.
+ c1 = 0.
+ c2 = 0.
+
+ ! integer
+ n = 0
+ mm = maxval (h(i1))
+ if (n /= k*k .or. mm /= 0) stop 1
+ n = 0
+ mm = maxval (h(i2))
+ if (n /= k*k .or. mm /= 0) stop 2
+ n = 0
+ mm = maxval (h(i1),m1)
+ if (n /= k*k .or. mm /= 0) stop 3
+ n = 0
+ mm = maxval (h(i2),m2)
+ if (n /= k*k .or. mm /= 0) stop 4
+
+ ! real
+ n = 0
+ mx = maxval (f(a1))
+ if (n /= k*k .or. mx /= 0) st
Re: [PATCH] Fortran: Added support for locality specs in DO CONCURRENT (Fortran 2018/23)
The first error message in my previous email led me to the following constraint: “*C1130* A *variable-name *that appears in a LOCAL or LOCAL_INIT *locality-spec *shall not have the ALLOCATABLE, INTENT (IN), or OPTIONAL attribute, shall not be of finalizable type, shall not have an allocatable ultimate component,...” My first thought was, "Holy guacamole. That seems like such a severe limitation that the feature seems almost useless." Fortuitously, however, it turns out that the code I sent a little while ago was missing one important feature from my intended use case: associate. As shown below, using associate eliminates the first error, but I'm still confused by the remaining error message. Are locality specifiers actually supported yet? Damian % cat locality.f90 program main implicit none integer pair integer :: mini_batch_size=1 real, allocatable, dimension(:,:) :: a, dcdb allocate(a(1,1)) allocate(dcdb(1,1)) associate(a_ => a, dcdb_ => dcdb) do concurrent (pair = 1:mini_batch_size) local(a_) reduce(+: dcdb_) a_ = 0. dcdb_ = 0. end do end associate end program % gfortran locality.f90 *locality.f90:12:71:* 12 | do concurrent (pair = 1:mini_batch_size) local(a_) reduce(+: dcdb_) | *1* *Error:* Sorry, LOCAL and LOCAL_INIT are not yet supported for ‘*do concurrent*’ constructs at *(1)* % gfortran --version GNU Fortran (GCC) 15.0.1 20250119 (experimental) Copyright (C) 2025 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Re: [PATCH] Fortran: Added support for locality specs in DO CONCURRENT (Fortran 2018/23)
I recently built the master branch of Iain's fork of gcc in order to test the support for locality specifiers. I have verified that the branch I built contains the commit mentioned in this email thread 20b8500cfa522ebe0fcf756d5b32816da7f904dd. The code below isn't designed to do anything useful -- just to make sure the syntax works. Could someone tell me why the second error message indicates that LOCAL is not supported yet? Also, the first error message is really interesting because if I'm really not allowed to have anything allocatable inside LOCAL(), then I'm going to have to go back to the drawing board regarding my intended use case. Any advice? Damian % cat locality.f90 program main implicit none integer pair integer :: mini_batch_size=1 real, allocatable, dimension(:,:) :: a, dcdb allocate(a(1,1)) allocate(dcdb(1,1)) do concurrent (pair = 1:mini_batch_size) local(a) reduce(+: dcdb) a = 0. dcdb = 0. end do end program % gfortran locality.f90 *locality.f90:8:50:* 8 | do concurrent (pair = 1:mini_batch_size) local(a) reduce(+: dcdb) | *1* *Error:* ALLOCATABLE attribute not permitted for ‘*a*’ in LOCAL locality-spec at *(1)* *locality.f90:8:67:* 8 | do concurrent (pair = 1:mini_batch_size) local(a) reduce(+: dcdb) | *1* *Error:* Sorry, LOCAL and LOCAL_INIT are not yet supported for ‘*do concurrent*’ constructs at *(1)* gfortran --version GNU Fortran (GCC) 15.0.1 20250119 (experimental) Copyright (C) 2025 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. On Mon, Jan 13, 2025 at 5:41 PM Jerry D wrote: > Committed as: > > commit 20b8500cfa522ebe0fcf756d5b32816da7f904dd (HEAD -> master, > origin/master, origin/HEAD) > Author: Anuj Mohite > Date: Mon Jan 13 16:28:57 2025 -0800 > > Fortran: Add LOCALITY support for DO_CONCURRENT > > This patch provided by Anuj Mohite as part of the GSoC > project. > It is modified slightly by Jerry DeLisle for minor formatting. > The patch provides front-end parsing of the LOCALITY specs in > DO_CONCURRENT and adds numerous test cases. > > gcc/fortran/ChangeLog: > > * dump-parse-tree.cc (show_code_node): Updated to use > c->ext.concur.forall_iterator instead of > c->ext.forall_iterator. > * frontend-passes.cc (index_interchange): Updated to > use c->ext.concur.forall_iterator instead of > c->ext.forall_iterator. > (gfc_code_walker): Likewise. > * gfortran.h (enum locality_type): Added new enum for > locality types > in DO CONCURRENT constructs. > * match.cc (match_simple_forall): Updated to use > new_st.ext.concur.forall_iterator instead of > new_st.ext.forall_iterator. > (gfc_match_forall): Likewise. > (gfc_match_do): Implemented support for matching DO > CONCURRENT locality > specifiers (LOCAL, LOCAL_INIT, SHARED, DEFAULT(NONE), and > REDUCE). > * parse.cc (parse_do_block): Updated to use > new_st.ext.concur.forall_iterator instead of > new_st.ext.forall_iterator. > * resolve.cc (struct check_default_none_data): Added struct > check_default_none_data. > (do_concur_locality_specs_f2023): New function to check > compliance > with F2023's C1133 constraint for DO CONCURRENT. > (check_default_none_expr): New function to check DEFAULT(NONE) > compliance. > (resolve_locality_spec): New function to resolve locality > specs. > (gfc_count_forall_iterators): Updated to use > code->ext.concur.forall_iterator. > (gfc_resolve_forall): Updated to use > code->ext.concur.forall_iterator. > * st.cc (gfc_free_statement): Updated to free locality > specifications > and use p->ext.concur.forall_iterator. > * trans-stmt.cc (gfc_trans_forall_1): Updated to use > code->ext.concur.forall_iterator. > > gcc/testsuite/ChangeLog: > > * gfortran.dg/do_concurrent_10.f90: New test. > * gfortran.dg/do_concurrent_8_f2018.f90: New test. > * gfortran.dg/do_concurrent_8_f2023.f90: New test. > * gfortran.dg/do_concurrent_9.f90: New test. > * gfortran.dg/do_concurrent_all_clauses.f90: New test. > * gfortran.dg/do_concurrent_basic.f90: New test. > * gfortran.dg/do_concurrent_constraints.f90: New test. > * gfortran.dg/do_concurrent_local_init.f90: New test. > * gfortran.dg/do_concurrent_locality_specs.f90: New test. > * gfortran.dg/do_concurrent_multiple_reduce.f90: New test. >
Re: [PATCH] Fortran: Added support for locality specs in DO CONCURRENT (Fortran 2018/23)
Hi Damian, looking into the code I neither find the keyword LOCAL nor REDUCE. The match routine also does not give any hint that those keywords are supported in a do concurrent loop. And here is the existing bug ticket: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101602 But I don't see a ticket for REDUCE yet. Sorry for the bad news. Regards, Andre On Wed, 22 Jan 2025 20:59:02 -0800 Damian Rouson wrote: > The first error message in my previous email led me to the following > constraint: > > “*C1130* A *variable-name *that appears in a LOCAL or LOCAL_INIT > *locality-spec *shall not have the ALLOCATABLE, INTENT (IN), or OPTIONAL > attribute, shall not be of finalizable type, shall not have an allocatable > ultimate component,...” > > My first thought was, "Holy guacamole. That seems like such > a severe limitation that the feature seems almost useless." Fortuitously, > however, it turns out that the code I sent a little while ago was missing > one important feature from my intended use case: associate. As shown > below, using associate eliminates the first error, but I'm still confused > by the remaining error message. Are locality specifiers actually supported > yet? > > Damian > > % cat locality.f90 > > program main > > implicit none > > integer pair > > integer :: mini_batch_size=1 > > > real, allocatable, dimension(:,:) :: a, dcdb > > > > allocate(a(1,1)) > > allocate(dcdb(1,1)) > > > associate(a_ => a, dcdb_ => dcdb) > > do concurrent (pair = 1:mini_batch_size) local(a_) reduce(+: dcdb_) > > a_ = 0. > > dcdb_ = 0. > > end do > > end associate > > > end program > > > % gfortran locality.f90 > > *locality.f90:12:71:* > > >12 | do concurrent (pair = 1:mini_batch_size) local(a_) reduce(+: > dcdb_) > > | > *1* > > *Error:* Sorry, LOCAL and LOCAL_INIT are not yet supported for ‘*do > concurrent*’ constructs at *(1)* > > > % gfortran --version > > GNU Fortran (GCC) 15.0.1 20250119 (experimental) > > Copyright (C) 2025 Free Software Foundation, Inc. > > This is free software; see the source for copying conditions. There is NO > > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. -- Andre Vehreschild * Email: vehre ad gmx dot de
