diff mbox

[fortran] PR 60392 wrong descriptor when passing a transposed array to a contiguous assumed shape dummy.

Message ID 531C95FB.2020003@sfr.fr
State New
Headers show

Commit Message

Mikael Morin March 9, 2014, 4:25 p.m. UTC 
Hello,

here is a fix for a wrong code issue, where we pass a descriptor with
broken bounds when the actual argument is a transposed array and the
dummy an assumed shape dummy.
The bug comes from the interaction of the transpose optimization,
which creates a descriptor with transposed bounds without copying the
data, and the contiguous optimization, which reuses the descriptor for
passing as argument after the call to internal_pack.
The attached patch makes a copy of the descriptor with the correct
bounds when a transposed scalarization is detected.

Regression-tested on x86_64-unknown-linux-gnu.
This is not a regression as far as I know, but quite a severe
wrong-code, albeit limited to the corner case of transpose and
assumed shape and contiguous.  OK for trunk/4.8/4.7 anyway ?


Mikael

PS: To not reproduce the same mistake as in the PR regarding the memory
representation of matrices, I have filled the matrices one element at a
time in the testcase.

Comments

Janus Weil March 11, 2014, 9:25 a.m. UTC | #1 
Hi Mikael,

> here is a fix for a wrong code issue, where we pass a descriptor with
> broken bounds when the actual argument is a transposed array and the
> dummy an assumed shape dummy.
> The bug comes from the interaction of the transpose optimization,
> which creates a descriptor with transposed bounds without copying the
> data, and the contiguous optimization, which reuses the descriptor for
> passing as argument after the call to internal_pack.
> The attached patch makes a copy of the descriptor with the correct
> bounds when a transposed scalarization is detected.
>
> Regression-tested on x86_64-unknown-linux-gnu.
> This is not a regression as far as I know, but quite a severe
> wrong-code, albeit limited to the corner case of transpose and
> assumed shape and contiguous.  OK for trunk/4.8/4.7 anyway ?

I would say it's ok for trunk at least. About the branches I'm not
sure. Maybe someone else can add an opinion here ...

Cheers,
Janus
Mikael Morin March 12, 2014, 9:19 p.m. UTC | #2 
Le 11/03/2014 10:25, Janus Weil a écrit :
>> Regression-tested on x86_64-unknown-linux-gnu.
>> This is not a regression as far as I know, but quite a severe
>> wrong-code, albeit limited to the corner case of transpose and
>> assumed shape and contiguous.  OK for trunk/4.8/4.7 anyway ?
> 
> I would say it's ok for trunk at least. About the branches I'm not
> sure. Maybe someone else can add an opinion here ...
> 
Now we are in stage4, so one could argue whether trunk should get a less
conservative treatment than the branches.

Anyway, I will apply to trunk by the end of the week, and leave the
branches untouched for now.

Thanks
Mikael
diff mbox

Patch

2014-03-09  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/60392
	* trans-array.c (gfc_conv_array_parameter): Don't reuse the descriptor
	if it has transposed dimensions.

2014-03-09  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/60392
	* gfortran.dg/transpose_4.f90: New test.

Index: trans-array.c
===================================================================
--- trans-array.c	(révision 208442)
+++ trans-array.c	(copie de travail)
@@ -7227,7 +7227,50 @@  gfc_conv_array_parameter (gfc_se * se, gfc_expr *
       else
 	{
 	  tmp = build_fold_indirect_ref_loc (input_location, desc);
-	  gfc_conv_descriptor_data_set (&se->pre, tmp, ptr);
+
+	  gfc_ss * ss = gfc_walk_expr (expr);
+	  if (!transposed_dims (ss))
+	    gfc_conv_descriptor_data_set (&se->pre, tmp, ptr);
+	  else
+	    {
+	      tree old_field, new_field;
+
+	      /* The original descriptor has transposed dims so we can't reuse
+		 it directly; we have to create a new one.  */
+	      tree old_desc = tmp;
+	      tree new_desc = gfc_create_var (TREE_TYPE (old_desc), "arg_desc");
+
+	      old_field = gfc_conv_descriptor_dtype (old_desc);
+	      new_field = gfc_conv_descriptor_dtype (new_desc);
+	      gfc_add_modify (&se->pre, new_field, old_field);
+
+	      old_field = gfc_conv_descriptor_offset (old_desc);
+	      new_field = gfc_conv_descriptor_offset (new_desc);
+	      gfc_add_modify (&se->pre, new_field, old_field);
+
+	      for (int i = 0; i < expr->rank; i++)
+		{
+		  old_field = gfc_conv_descriptor_dimension (old_desc,
+			gfc_rank_cst[get_array_ref_dim_for_loop_dim (ss, i)]);
+		  new_field = gfc_conv_descriptor_dimension (new_desc,
+			gfc_rank_cst[i]);
+		  gfc_add_modify (&se->pre, new_field, old_field);
+		}
+
+	      if (gfc_option.coarray == GFC_FCOARRAY_LIB
+		  && GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (old_desc))
+		  && GFC_TYPE_ARRAY_AKIND (TREE_TYPE (old_desc))
+		     == GFC_ARRAY_ALLOCATABLE)
+		{
+		  old_field = gfc_conv_descriptor_token (old_desc);
+		  new_field = gfc_conv_descriptor_token (new_desc);
+		  gfc_add_modify (&se->pre, new_field, old_field);
+		}
+
+	      gfc_conv_descriptor_data_set (&se->pre, new_desc, ptr);
+	      se->expr = gfc_build_addr_expr (NULL_TREE, new_desc);
+	    }
+	  gfc_free_ss (ss);
 	}
 
       if (gfc_option.rtcheck & GFC_RTCHECK_ARRAY_TEMPS)