[Patch, fortran, pr65548, 2nd take, v3] [5/6 Regression] gfc_conv_procedure_call

[Mikael Morin]

Hello,

Le 30/04/2015 15:07, Andre Vehreschild a écrit :
> Hi all,
> 
> this is just a service release. I encountered that the new testcase in the
> previous release included the testcase of the initial patch, that is
> already on trunk. I therefore replaced the testcase allocate_with_source_5.f90
> by allocate_with_source_6.f90 (the extended testcase). Besides this there is no
> difference inbetween this and the patch in:
> 
> https://gcc.gnu.org/ml/fortran/2015-04/msg00121.html
> 
> Sorry for the mess. For a description of the original patches scope see below.
> 
> Bootstraps and regtests ok on x86_64-linux-gnu/F21.
> 
> Ok for trunk?
> 
> Regards,
> 	Andre
> 
> On Wed, 29 Apr 2015 14:31:01 +0200
> Andre Vehreschild <vehre@gmx.de> wrote:
> 
>> Hi all,
>>
>> after the first patch to fix the issue reported in the pr, some more issues
>> were reported, which are now fixed by this new patch, aka the 2nd take.
>>
>> The patch modifies the gfc_trans_allocate() in order to pre-evaluate all
>> source= expressions. It no longer rejects array valued source= expressions,
>> but just uses gfc_conv_expr_descriptor () for most of them. Furthermore, is
>> the allocate now again able to allocate arrays of strings. This feature
>> previously slipped my attention.
>>
>> Although the reporter has not yet reported, that the patch fixes his issue, I
>> like to post it for review, because there are more patches in my pipeline,
>> that depend on this one. 
>>
>> Bootstraps and regtests ok on x86_64-linux-gnu/F21.
>>
>> Ok, for trunk?
>>
questions below
> 
> 
> *** trans-stmt.c	2015-05-12 14:42:17.882108651 +0200
> --- trans-stmt.c.modif	2015-05-12 14:42:11.300108561 +0200
> ***************
> *** 5205,5213 ****
>   	      /* In all other cases evaluate the expr3 and create a
>   		 temporary.  */
>   	      gfc_init_se (&se, NULL);
>   	      if (code->expr3->rank != 0
> ! 		  && code->expr3->expr_type == EXPR_FUNCTION
> ! 		  && code->expr3->value.function.isym)
>   		gfc_conv_expr_descriptor (&se, code->expr3);
>   	      else
>   		gfc_conv_expr_reference (&se, code->expr3);
> --- 5198,5222 ----
>   	  /* In all other cases evaluate the expr3 and create a
>   		 temporary.  */
>   	  gfc_init_se (&se, NULL);
> + 	  /* For more complicated expression, the decision when to get the
> + 	     descriptor and when to get a reference is depending on more
> + 	     conditions.  The descriptor is only retrieved for functions
> + 	     that are intrinsic, elemental user-defined and known, or neither
> + 	     of the two, or are a class or type, that has a not deferred type
> + 	     array_spec.  */
>   	  if (code->expr3->rank != 0
> ! 	      && (code->expr3->expr_type != EXPR_FUNCTION
> ! 		  || code->expr3->value.function.isym
> ! 		  || (code->expr3->value.function.esym &&
> ! 		      code->expr3->value.function.esym->attr.elemental)
> ! 		  || (!code->expr3->value.function.isym
> ! 		      && !code->expr3->value.function.esym)
> ! 		  || (code->expr3->ts.type == BT_DERIVED
> ! 		      && code->expr3->ts.u.derived->as
> ! 		      && code->expr3->ts.u.derived->as->type != AS_DEFERRED)
> ! 		  || (code->expr3->ts.type == BT_CLASS
> ! 		      && CLASS_DATA (code->expr3)->as
> ! 		      && CLASS_DATA (code->expr3)->as->type != AS_DEFERRED)))
>   	    gfc_conv_expr_descriptor (&se, code->expr3);
>   	  else
>   	    gfc_conv_expr_reference (&se, code->expr3);
What is the rationale for choosing between gfc_conv_expr_descriptor and
gfc_conv_expr_reference?
Is it contiguous array vs non-contiguous or needing an evaluation?
For example why not use gfc_conv_expr_descriptor for derived type arrays?

> ***************
> *** 5646,5659 ****
>   	    }
>   	  else if (code->expr3->ts.type == BT_CHARACTER)
>   	    {
> ! 	      tmp = INDIRECT_REF_P (se.expr) ?
>   			se.expr :
>   			build_fold_indirect_ref_loc (input_location,
>   						     se.expr);
> ! 	      gfc_trans_string_copy (&block, al_len, tmp,
> ! 				     code->expr3->ts.kind,
> ! 				     expr3_len, expr3,
> ! 				     code->expr3->ts.kind);
>   	      tmp = NULL_TREE;
>   	    }
>   	  else if (al->expr->ts.type == BT_CLASS)
> --- 5658,5707 ----
>   	    }
>   	  else if (code->expr3->ts.type == BT_CHARACTER)
>   	    {
> ! 	      tree dst, src, dlen, slen;
> ! 	      /* For arrays of char arrays, a ref to the data component still
> ! 		 needs to be added, because se.expr upto now only contains the
> ! 		 descritor.  */
> ! 	      if (expr->ref && se.expr && TREE_TYPE (se.expr) != NULL_TREE
> ! 		  && GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (se.expr)))
> ! 		{
> ! 		  dst = gfc_conv_array_data (se.expr);
> ! 		  src = gfc_conv_array_data (expr3);
> ! 		  /* For CHARACTER (len=string_length), dimension (nelems)
> ! 		     compute the total length of the string to copy.  */
> ! 		  if (nelems)
> ! 		    {
> ! 		      dlen = fold_build2_loc (input_location, MULT_EXPR,
> ! 					      size_type_node,
> ! 					      fold_convert (size_type_node,
> ! 							    se.string_length),
> ! 					      fold_convert (size_type_node,
> ! 							    nelems));
> ! 		      slen = fold_build2_loc (input_location, MULT_EXPR,
> ! 					      size_type_node,
> ! 					      fold_convert (size_type_node,
> ! 							    expr3_len),
> ! 					      fold_convert (size_type_node,
> ! 							    nelems));
> ! 		    }
> ! 		  else
> ! 		    {
> ! 		      dlen = se.string_length;
> ! 		      slen = expr3_len;
> ! 		    }
> ! 		}
> ! 	      else
> ! 		{
> ! 		  dst = INDIRECT_REF_P (se.expr) ?
>   			se.expr :
>   			build_fold_indirect_ref_loc (input_location,
>   						     se.expr);
> ! 		  src = expr3;
> ! 		  dlen = al_len;
> ! 		  slen = expr3_len;
> ! 		}
> ! 	      gfc_trans_string_copy (&block, dlen, dst, code->expr3->ts.kind,
> ! 				     slen, src, code->expr3->ts.kind);
>   	      tmp = NULL_TREE;
>   	    }
>   	  else if (al->expr->ts.type == BT_CLASS)
This seems to assume that the array is contiguous.
Can't we just fall  back to the default case for characters?

The rest looks good.

Mikael