[fortran,PR44672,v6,F08] ALLOCATE with SOURCE and no array-spec

Hi Mikael,

comments inline below:

On Thu, 28 May 2015 20:06:57 +0200
Mikael Morin <mikael.morin@sfr.fr> wrote:

> Le 28/05/2015 17:29, Andre Vehreschild a écrit :
> > *************** resolve_allocate_expr (gfc_expr *e, gfc_
> > *** 7103,7112 ****
> > --- 7103,7123 ----
> >     if (!ref2 || ref2->type != REF_ARRAY || ref2->u.ar.type == AR_FULL
> >         || (dimension && ref2->u.ar.dimen == 0))
> >       {
> > +       /* F08:C633.  */
> > +       if (code->expr3)
> > + 	{
> > + 	  if (!gfc_notify_std (GFC_STD_F2008, "Array specification
> > required "
> > + 			       "in ALLOCATE statement at %L", &e->where))
> > + 	    goto failure;
> > + 	  *array_alloc_wo_spec = true;
> > + 	}
> > +       else
> > + 	{
> >   	  gfc_error ("Array specification required in ALLOCATE statement "
> >   		     "at %L", &e->where);
> >   	  goto failure;
> >   	}
> > +     }
> >   
> >     /* Make sure that the array section reference makes sense in the
> >        context of an ALLOCATE specification.  */
> I think we can be a little be more user friendly with the gfc_notify_std
> error message.
> Something like:
> ALLOCATE without array spec at %L
> ALLOCATE with array bounds determined from SOURCE or MOLD at %L

I didn't want to mess with the error messages to prevent issues for
translations. So how is the policy on this? 

> > *************** gfc_array_init_size (tree descriptor, in
> > *** 5044,5053 ****
> >   	 lower == NULL    => lbound = 1, ubound = upper[n]
> >   	 upper[n] = NULL  => lbound = 1, ubound = lower[n]
> >   	 upper[n] != NULL => lbound = lower[n], ubound = upper[n]  */
> > -       ubound = upper[n];
> >   
> >         /* Set lower bound.  */
> >         gfc_init_se (&se, NULL);
> >         if (lower == NULL)
> >   	se.expr = gfc_index_one_node;
> >         else
> > --- 5050,5063 ----
> >   	 lower == NULL    => lbound = 1, ubound = upper[n]
> >   	 upper[n] = NULL  => lbound = 1, ubound = lower[n]
> >   	 upper[n] != NULL => lbound = lower[n], ubound = upper[n]  */
> >   
> >         /* Set lower bound.  */
> >         gfc_init_se (&se, NULL);
> > +       if (expr3_desc != NULL_TREE)
> > + 	se.expr = gfc_index_one_node;
> > +       else
> > + 	{
> > + 	  ubound = upper[n];
> >   	  if (lower == NULL)
> >   	    se.expr = gfc_index_one_node;
> >   	  else
> > *************** gfc_array_init_size (tree descriptor, in
> > *** 5064,5069 ****
> > --- 5074,5080 ----
> >   		  ubound = lower[n];
> >   		}
> >   	    }
> > + 	}
> >         gfc_conv_descriptor_lbound_set (descriptor_block, descriptor,
> >   				      gfc_rank_cst[n], se.expr);
> >         conv_lbound = se.expr;
> You can avoid reindenting if the ubound = upper[n] statement is kept at
> its original place.

Fixed.

> > *************** gfc_array_init_size (tree descriptor, in
> > *** 5076,5085 ****
> >   
> >         /* Set upper bound.  */
> >         gfc_init_se (&se, NULL);
> >         gcc_assert (ubound);
> >         gfc_conv_expr_type (&se, ubound, gfc_array_index_type);
> >         gfc_add_block_to_block (pblock, &se.pre);
> > ! 
> >         gfc_conv_descriptor_ubound_set (descriptor_block, descriptor,
> >   				      gfc_rank_cst[n], se.expr);
> >         conv_ubound = se.expr;
> > --- 5087,5111 ----
> >   
> >         /* Set upper bound.  */
> >         gfc_init_se (&se, NULL);
> > +       if (expr3_desc != NULL_TREE)
> > + 	{
> > + 	  /* Set the upper bound to be (desc.ubound - desc.lbound)+ 1.  */
> > + 	  tmp = fold_build2_loc (input_location, MINUS_EXPR,
> > + 				 gfc_array_index_type,
> > + 				 gfc_conv_descriptor_ubound_get (
> > + 				   expr3_desc, gfc_rank_cst[n]),
> > + 				 gfc_conv_descriptor_lbound_get (
> > + 				   expr3_desc, gfc_rank_cst[n]));
> > + 	  se.expr = fold_build2_loc (input_location, PLUS_EXPR,
> > + 				     gfc_array_index_type, tmp,
> > + 				     gfc_index_one_node);
> > + 	}
> > +       else
> > + 	{
> >   	  gcc_assert (ubound);
> >   	  gfc_conv_expr_type (&se, ubound, gfc_array_index_type);
> >   	  gfc_add_block_to_block (pblock, &se.pre);
> > ! 	}
> >         gfc_conv_descriptor_ubound_set (descriptor_block, descriptor,
> >   				      gfc_rank_cst[n], se.expr);
> >         conv_ubound = se.expr;
> Your one-based-ness problem was here, wasn't it?

Correct.

> I would rather copy directly lbound and ubound from expr3_desc to
> descriptor.

It was that way in the previous version of the patch, which does *not* work any
longer. When gfc_trans_allocate () is responsible for the creating a temporary
variable for the source=-expression, then it does so using zero based
expressions. 

> If the source has non-one-based bounds, the above would produce wrong
> bounds.

Counterexample? Note, the expr3_desc is guaranteed to be an artificial variable
created by conv_expr_descriptor, aka zero-based.

<snipp>

> > *************** gfc_trans_allocate (gfc_code * code)
> > *** 5229,5235 ****
> >   	    }
> >   	  else
> >   	    tmp = se.expr;
> > ! 	  if (!code->expr3->mold)
> >   	    expr3 = tmp;
> >   	  else
> >   	    expr3_tmp = tmp;
> > --- 5240,5248 ----
> >   	    }
> >   	  else
> >   	    tmp = se.expr;
> > ! 	  if (code->ext.alloc.arr_spec_from_expr3)
> > ! 	    expr3_desc = tmp;
> > ! 	  else if (!code->expr3->mold)
> >   	    expr3 = tmp;
> >   	  else
> >   	    expr3_tmp = tmp;
> Couldn't expr3 be reused?
> We had code->expr3, expr3, expr3rhs, and now this is adding expr3_desc,
> and (below) inexpr3. :-(

Of course can we use just two variables for all expressions. I have removed the
expr3_tmp, inexpr3 and expr3_desc and introduced a e3_is enumeration, which
stores which kind the expr3 is, aka unset, source, mold, desc. This makes the
code simpler at some places.

Attached is a new version of the patch. This one fails
allocate_with_source_3.f90 on runtime, where I don't see the issue currently.
May be you have some luck and time. If not I will investigate on Monday.

Regards,
	Andre

[fortran,PR44672,v6,F08] ALLOCATE with SOURCE and no array-spec

Commit Message

Comments

Patch