Re: [PATCH] Fortran: intrinsic MERGE shall use all its arguments [PR107874]
Dear all, I've committed an obvious fix for the testcase to avoid recursive I/O, as it did cause a hang on some systems: https://gcc.gnu.org/g:36a4ee406b95ae24a59b8b3f8ebe29af6fd5261e Confirmed by Jerry that this resolves his issue. See also attached. Thanks, Harald Am 29.11.22 um 09:08 schrieb Paul Richard Thomas via Gcc-patches: Hi Harald, It looks good to me. Thanks to you and Steve for the patch. Paul On Mon, 28 Nov 2022 at 20:05, Harald Anlauf via Fortran wrote: Dear all, as reported, the Fortran standard requires all actual argument expressions to be evaluated (e.g. F2018:15.5.3). There were two cases for intrinsic MERGE where we failed to do so: - non-constant mask; Steve provided the patch - constant scalar mask; we need to be careful to simplify only if the argument on the "other" path is known to be constant so that it does not have side-effects and can be immediately removed. The latter change needed a correction of a sub-test of testcase merge_init_expr_2.f90, which should not have been simplified the way the original author assumed. I decided to modify the test in such way that simplification is valid and provides the expect pattern. Regtested on x86_64-pc-linux-gnu. OK for mainline? Thanks, Harald From 36a4ee406b95ae24a59b8b3f8ebe29af6fd5261e Mon Sep 17 00:00:00 2001 From: Harald Anlauf Date: Fri, 2 Dec 2022 22:30:16 +0100 Subject: [PATCH] Fortran: intrinsic MERGE shall use all its arguments [PR107874] gcc/testsuite/ChangeLog: PR fortran/107874 * gfortran.dg/merge_1.f90: Avoid recursive I/O. --- gcc/testsuite/gfortran.dg/merge_1.f90 | 22 +++--- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/gcc/testsuite/gfortran.dg/merge_1.f90 b/gcc/testsuite/gfortran.dg/merge_1.f90 index abbc2276b1c..437b13a8d3f 100644 --- a/gcc/testsuite/gfortran.dg/merge_1.f90 +++ b/gcc/testsuite/gfortran.dg/merge_1.f90 @@ -7,32 +7,40 @@ program testmerge9 integer :: i logical :: x(2) = (/.true., .false./) logical :: called(2) + logical :: y ! At run-time all arguments shall be evaluated do i = 1,2 called = .false. - print *, merge (tstuff(), fstuff(), x(i)) + y = merge (tstuff(), fstuff(), x(i)) + print *, y if (any (.not. called)) stop 1 end do ! Compile-time simplification shall not drop non-constant args called = .false. - print *, merge (tstuff(),fstuff(),.true.) + y = merge (tstuff(),fstuff(),.true.) + print *, y if (any (.not. called)) stop 2 called = .false. - print *, merge (tstuff(),fstuff(),.false.) + y = merge (tstuff(),fstuff(),.false.) + print *, y if (any (.not. called)) stop 3 called = .false. - print *, merge (tstuff(),.false.,.true.) + y = merge (tstuff(),.false.,.true.) + print *, y if (any (called .neqv. [.true.,.false.])) stop 4 called = .false. - print *, merge (tstuff(),.false.,.false.) + y = merge (tstuff(),.false.,.false.) + print *, y if (any (called .neqv. [.true.,.false.])) stop 5 called = .false. - print *, merge (.true.,fstuff(),.true.) + y = merge (.true.,fstuff(),.true.) + print *, y if (any (called .neqv. [.false.,.true.])) stop 6 called = .false. - print *, merge (.true.,fstuff(),.false.) + y = merge (.true.,fstuff(),.false.) + print *, y if (any (called .neqv. [.false.,.true.])) stop 7 contains logical function tstuff() -- 2.35.3
Re: [PATCH] Fortran: intrinsic MERGE shall use all its arguments [PR107874]
Hi Harald, It looks good to me. Thanks to you and Steve for the patch. Paul On Mon, 28 Nov 2022 at 20:05, Harald Anlauf via Fortran wrote: > Dear all, > > as reported, the Fortran standard requires all actual argument > expressions to be evaluated (e.g. F2018:15.5.3). > > There were two cases for intrinsic MERGE where we failed to do so: > > - non-constant mask; Steve provided the patch > > - constant scalar mask; we need to be careful to simplify only if > the argument on the "other" path is known to be constant so that > it does not have side-effects and can be immediately removed. > > The latter change needed a correction of a sub-test of testcase > merge_init_expr_2.f90, which should not have been simplified > the way the original author assumed. I decided to modify the > test in such way that simplification is valid and provides > the expect pattern. > > Regtested on x86_64-pc-linux-gnu. OK for mainline? > > Thanks, > Harald > -- "If you can't explain it simply, you don't understand it well enough" - Albert Einstein
[PATCH] Fortran: intrinsic MERGE shall use all its arguments [PR107874]
Dear all, as reported, the Fortran standard requires all actual argument expressions to be evaluated (e.g. F2018:15.5.3). There were two cases for intrinsic MERGE where we failed to do so: - non-constant mask; Steve provided the patch - constant scalar mask; we need to be careful to simplify only if the argument on the "other" path is known to be constant so that it does not have side-effects and can be immediately removed. The latter change needed a correction of a sub-test of testcase merge_init_expr_2.f90, which should not have been simplified the way the original author assumed. I decided to modify the test in such way that simplification is valid and provides the expect pattern. Regtested on x86_64-pc-linux-gnu. OK for mainline? Thanks, Harald From 0f6058937c04a7af5e6dcfa173648149c24f08df Mon Sep 17 00:00:00 2001 From: Harald Anlauf Date: Mon, 28 Nov 2022 20:43:02 +0100 Subject: [PATCH] Fortran: intrinsic MERGE shall use all its arguments [PR107874] gcc/fortran/ChangeLog: PR fortran/107874 * simplify.cc (gfc_simplify_merge): When simplifying MERGE with a constant scalar MASK, ensure that arguments TSOURCE and FSOURCE are either constant or will be evaluated. * trans-intrinsic.cc (gfc_conv_intrinsic_merge): Evaluate arguments before generating conditional expression. gcc/testsuite/ChangeLog: PR fortran/107874 * gfortran.dg/merge_init_expr_2.f90: Adjust code to the corrected simplification. * gfortran.dg/merge_1.f90: New test. Co-authored-by: Steven G. Kargl --- gcc/fortran/simplify.cc | 17 ++- gcc/fortran/trans-intrinsic.cc| 3 ++ gcc/testsuite/gfortran.dg/merge_1.f90 | 49 +++ .../gfortran.dg/merge_init_expr_2.f90 | 3 +- 4 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gfortran.dg/merge_1.f90 diff --git a/gcc/fortran/simplify.cc b/gcc/fortran/simplify.cc index 9c2fea8c5f2..b6184181f26 100644 --- a/gcc/fortran/simplify.cc +++ b/gcc/fortran/simplify.cc @@ -4913,7 +4913,22 @@ gfc_simplify_merge (gfc_expr *tsource, gfc_expr *fsource, gfc_expr *mask) if (mask->expr_type == EXPR_CONSTANT) { - result = gfc_copy_expr (mask->value.logical ? tsource : fsource); + /* The standard requires evaluation of all function arguments. + Simplify only when the other dropped argument (FSOURCE or TSOURCE) + is a constant expression. */ + if (mask->value.logical) + { + if (!gfc_is_constant_expr (fsource)) + return NULL; + result = gfc_copy_expr (tsource); + } + else + { + if (!gfc_is_constant_expr (tsource)) + return NULL; + result = gfc_copy_expr (fsource); + } + /* Parenthesis is needed to get lower bounds of 1. */ result = gfc_get_parentheses (result); gfc_simplify_expr (result, 1); diff --git a/gcc/fortran/trans-intrinsic.cc b/gcc/fortran/trans-intrinsic.cc index bb938026828..93426981bac 100644 --- a/gcc/fortran/trans-intrinsic.cc +++ b/gcc/fortran/trans-intrinsic.cc @@ -7557,6 +7557,9 @@ gfc_conv_intrinsic_merge (gfc_se * se, gfc_expr * expr) &se->pre); se->string_length = len; } + tsource = gfc_evaluate_now (tsource, &se->pre); + fsource = gfc_evaluate_now (fsource, &se->pre); + mask = gfc_evaluate_now (mask, &se->pre); type = TREE_TYPE (tsource); se->expr = fold_build3_loc (input_location, COND_EXPR, type, mask, tsource, fold_convert (type, fsource)); diff --git a/gcc/testsuite/gfortran.dg/merge_1.f90 b/gcc/testsuite/gfortran.dg/merge_1.f90 new file mode 100644 index 000..abbc2276b1c --- /dev/null +++ b/gcc/testsuite/gfortran.dg/merge_1.f90 @@ -0,0 +1,49 @@ +! { dg-do run } +! PR fortran/107874 - merge not using all its arguments +! Contributed by John Harper + +program testmerge9 + implicit none + integer :: i + logical :: x(2) = (/.true., .false./) + logical :: called(2) + + ! At run-time all arguments shall be evaluated + do i = 1,2 + called = .false. + print *, merge (tstuff(), fstuff(), x(i)) + if (any (.not. called)) stop 1 + end do + + ! Compile-time simplification shall not drop non-constant args + called = .false. + print *, merge (tstuff(),fstuff(),.true.) + if (any (.not. called)) stop 2 + called = .false. + print *, merge (tstuff(),fstuff(),.false.) + if (any (.not. called)) stop 3 + called = .false. + print *, merge (tstuff(),.false.,.true.) + if (any (called .neqv. [.true.,.false.])) stop 4 + called = .false. + print *, merge (tstuff(),.false.,.false.) + if (any (called .neqv. [.true.,.false.])) stop 5 + called = .false. + print *, merge (.true.,fstuff(),.true.) + if (any (called .neqv. [.false.,.true.])) stop 6 + called = .false. + print *, merge (.true.,fstuff(),.false.) + if (any (called .neqv. [.false.,.true.])) stop 7 +contains + logical function tstuff() +print *,'tstuff' +tstuff = .true. +called(1) = .true. + end func