[PATCH] Fortran: fix PR103390, ICE in gimplification

2022-01-02 Thread Sandra Loosemore
This patch is for PR103390.  For background on this issue, the Fortran 
standard requires that, when passing a non-contiguous array from Fortran 
to a BIND(C) function with the CONTIGUOUS attribute on the corresponding 
dummy argument, the compiler has to arrange for it to be copied to/from 
a contiguous temporary.  The ICE was happening because the front end was 
attempting to copy out to an array-valued expression that isn't an 
lvalue, and producing invalid code.


I poked around at several related examples (included as test cases in 
the patch) and realized that it should not be doing any copying at all 
here, since the expression result already was being put in a contiguous 
temporary.  And, besides the invalid code on copy-out, in some cases it 
was generating multiple copies of the code to compute the expression on 
copy-in.  :-S


Both parts of the patch seem to be necessary to fix all the test cases. 
Tobias pointed me in this direction when I discussed it with him a few 
weeks ago so I hope I got it right.


OK to check in?  It regression-tests fine on x86_64.

-Sandra
commit 3a5e4f3a14b4265ee6f92dd724cbae9103d38d4b
Author: Sandra Loosemore 
Date:   Wed Dec 29 16:44:14 2021 -0800

Fortran: Fix array copy-in/copy-out for BIND(C) functions [PR103390]

The Fortran front end was generating invalid code for the array
copy-out after a call to a BIND(C) function for a dummy with the
CONTIGUOUS attribute when the actual argument was a call to the SHAPE
intrinsic or other array expressions that are not lvalues.  It was
also generating code to evaluate the argument expression multiple
times on copy-in.  This patch teaches it to recognize that a copy is
not needed in these cases.

2022-01-02  Sandra Loosemore  

	PR fortran/103390

	gcc/fortran/
	* expr.c (gfc_is_simply_contiguous): Make it smarter about
	function calls.
	* trans-expr.c (gfc_conv_gfc_desc_to_cfi_desc): Do not generate
	copy loops for array expressions that are not "variables" (lvalues).

	gcc/testsuite/
	* gfortran.dg/c-interop/pr103390-1.f90: New.
	* gfortran.dg/c-interop/pr103390-2.f90: New.
	* gfortran.dg/c-interop/pr103390-3.f90: New.
	* gfortran.dg/c-interop/pr103390-4.f90: New.
	* gfortran.dg/c-interop/pr103390-6.f90: New.
	* gfortran.dg/c-interop/pr103390-7.f90: New.
	* gfortran.dg/c-interop/pr103390-8.f90: New.
	* gfortran.dg/c-interop/pr103390-9.f90: New.

diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c
index c1258e0..a0129a3 100644
--- a/gcc/fortran/expr.c
+++ b/gcc/fortran/expr.c
@@ -5883,8 +5883,16 @@ gfc_is_simply_contiguous (gfc_expr *expr, bool strict, bool permit_element)
 
   if (expr->expr_type == EXPR_FUNCTION)
 {
-  if (expr->value.function.esym)
-	return expr->value.function.esym->result->attr.contiguous;
+  if (expr->value.function.isym)
+	/* TRANPOSE is the only intrinsic that may return a
+	   non-contiguous array.  It's treated as a special case in
+	   gfc_conv_expr_descriptor too.  */
+	return (expr->value.function.isym->id != GFC_ISYM_TRANSPOSE);
+  else if (expr->value.function.esym)
+	/* Only a pointer to an array without the contiguous attribute
+	   can be non-contiguous as a result value.  */
+	return (expr->value.function.esym->result->attr.contiguous
+		|| !expr->value.function.esym->result->attr.pointer);
   else
 	{
 	  /* Type-bound procedures.  */
diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
index 80c669f..10e1e37 100644
--- a/gcc/fortran/trans-expr.c
+++ b/gcc/fortran/trans-expr.c
@@ -5536,13 +5536,17 @@ gfc_conv_gfc_desc_to_cfi_desc (gfc_se *parmse, gfc_expr *e, gfc_symbol *fsym)
 {
   /* If the actual argument can be noncontiguous, copy-in/out is required,
 	 if the dummy has either the CONTIGUOUS attribute or is an assumed-
-	 length assumed-length/assumed-size CHARACTER array.  */
+	 length assumed-length/assumed-size CHARACTER array.  This only
+	 applies if the actual argument is a "variable"; if it's some
+	 non-lvalue expression, we are going to evaluate it to a
+	 temporary below anyway.  */
   se.force_no_tmp = 1;
   if ((fsym->attr.contiguous
 	   || (fsym->ts.type == BT_CHARACTER && !fsym->ts.u.cl->length
 	   && (fsym->as->type == AS_ASSUMED_SIZE
 		   || fsym->as->type == AS_EXPLICIT)))
-	  && !gfc_is_simply_contiguous (e, false, true))
+	  && !gfc_is_simply_contiguous (e, false, true)
+	  && gfc_expr_is_variable (e))
 	{
 	  bool optional = fsym->attr.optional;
 	  fsym->attr.optional = 0;
@@ -6841,6 +6845,8 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
 	 fsym->attr.pointer);
 		}
 	  else
+		/* This is where we introduce a temporary to store the
+		   result of a non-lvalue array expression.  */
 		gfc_conv_array_parameter (&parmse, e, nodesc_arg, fsym,
 	  sym->name, NULL);
 
diff --git a/gcc/testsuite/gfortran.dg/c-interop/pr103390-1.f90 b/gcc/testsuite/gfortran.dg/c-interop/p

Re: [PATCH] Fortran: fix PR103390, ICE in gimplification

2022-01-02 Thread Harald Anlauf via Gcc-patches

Hi Sandra,

Am 02.01.22 um 19:32 schrieb Sandra Loosemore:

This patch is for PR103390.  For background on this issue, the Fortran
standard requires that, when passing a non-contiguous array from Fortran
to a BIND(C) function with the CONTIGUOUS attribute on the corresponding
dummy argument, the compiler has to arrange for it to be copied to/from
a contiguous temporary.  The ICE was happening because the front end was
attempting to copy out to an array-valued expression that isn't an
lvalue, and producing invalid code.

I poked around at several related examples (included as test cases in
the patch) and realized that it should not be doing any copying at all
here, since the expression result already was being put in a contiguous
temporary.  And, besides the invalid code on copy-out, in some cases it
was generating multiple copies of the code to compute the expression on
copy-in.  :-S

Both parts of the patch seem to be necessary to fix all the test cases.
Tobias pointed me in this direction when I discussed it with him a few
weeks ago so I hope I got it right.

OK to check in?  It regression-tests fine on x86_64.


LGTM.

There are a few really minor things to improve:

+   /* TRANPOSE is the only intrinsic that may return a

s/TRANPOSE/TRANSPOSE/

+! We only expect one loop before the call, to fill in the contigous

s/contigous/contiguous/

+! { dg-final { scan-tree-dump-times "contiguous\\.\[0-9\]+" 0
"original" } }

There is a shorter, slightly shorter form for absence of a pattern:

! { dg-final { scan-tree-dump-not "contiguous\\.\[0-9\]+" "original" } }


-Sandra


Thanks for the patch!