Re: [PATCH] Fortran: ICE with elemental and dummy argument with VALUE attribute [PR107819]

2022-11-28 Thread Mikael Morin

Le 27/11/2022 à 21:32, Harald Anlauf via Fortran a écrit :

Dear Fortranners,

in dependency checking of arguments of elemental prodecures
we should treat dummy arguments with the value attribute as
implicitly having intent(in).  This is simple and obvious.

The PR by Gerhard provides a series of testcases that are
either valid (like the one in the attached patch), or
arguably non-conforming.  The issue is related to the
standard prescribing a temporary (in standardese language)
for the argument with the value attribute, while the
elemental attribute prescribes an application order.

Playing with other compiler brands, there seemed to be an
obvious discrepancy between NAG and Intel on the one side
and Intel on the other.  Steve Lionel attributed this to
non-conformance for the discussed case (see link in PR).

I therefore decided to only use a conforming testcase
for the testsuite, as this is sufficient to check for
the fix for the ICE.

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


Yes.
Thanks.


[PATCH] Fortran: intrinsic MERGE shall use all its arguments [PR107874]

2022-11-28 Thread Harald Anlauf via Fortran
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 function tstuff
+
+  logical function fstuff()
+print *,'fstuff'
+fs