This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH,gfortran] Fix PR 16222


On Wed, Dec 01, 2004 at 09:22:51PM +0000, Paul Brook wrote:
> On Sunday 28 November 2004 00:51, Steve Kargl wrote:
> > The attached patch fixes PR 16222.  Bootstrapped
> > and regression tested on i386-unknown-freebsd6.0.
> >
> > 2004-11-24  Steven G. Kargl  <kargls@comcast.net>
> >
> >         PR 16222
> >         * gfortran.h (gfc_resolve_do_iterator): Add prototype
> >         * resolve.c (gfc_resolve_do_iterator): New function; use it
> >         * trans-stmt.c (gfc_trans_do): Allow REAL iterator
> 
> Duplicating gfc_resolve_iterator seems a bad idea.
> 
> I think it would be better to add an extra boolean argument to 
> gfc_resolve_iterator which says whether read arguments are allowed.
> You may also want to factor out the individual variable checks into a common 
> routine.

I've introduced the two defines INTEGER_ONLY and REAL_OK and
updated gfc_resolve_iterator to permit REAL iterators for
DO loop variables.  Other uses of gfc_resolve_iterator use
INTEGER_ONLY.

> Are you deliberately allowing a mixture of integer and real types?

This has been fixed in gfc_resolve_iterator where I use gfc_convert_type
to force start, end, and step to the type and kind type parameter of
the DO loop variable.

> This seems like it deserves at least a -Wsuprising/-Wconversion warning.

I did not put in these options.  If you really want them, I'll do it.

> You should also update the comment(s) in gfc_trans_do. Remove the TODO, and 
> mention how we handle this case - make the conversion explicit in the 
> pseudocode.

I removed the TODO, but did not add a comment about the conversion
because the conversion is actually done resolve.c.

> >    /* Decrement the loop count.  */
> > +  if (TREE_CODE (type) == INTEGER_TYPE)
> >    tmp = build2 (MINUS_EXPR, type, count, gfc_index_one_node);
> > +  else
> > +    tmp = build2 (MINUS_EXPR, gfc_array_index_type, count, 
> gfc_index_one_node);
> >    gfc_add_modify_expr (&body, count, tmp);
> 
> The existing code is wrong, and your code is overly complicated. Try:
> 
> tree count_one = build_int_cst (TREE_TYPE (count), 1);
> ...
> tmp = build2 (MINUS_EXPR, TREE_TYPE (count), count, count_one);

I don't follow you here.  This is probably due to my lack of
understanding of the tree-ssa stuff and the backend.  Can you 
look at what I did and see if it's accept.

I'll note I did not go to great pains to try to guess at what 
the floating pointing iterator means.  To give, you a taste
of why FP iterators are bad.  These programs produce different
results:

      program do1
      real x, xmin, xmax, dx
      xmin = 0.
      xmax = 1.
      dx = 0.1
      do 1 x = xmin, xmax, dx
         print *, x
1     continue
      end


      program do1
      real x
      do 1 x = 0, 1, 0.1
         print *, x
1     continue
      end

Bootstrapped and regression tested with no new regressions on
i386-unknown-freebsd6.0.


2004-12-04  Steven G. Kargl  <kargls@comcast.net>

	* gfortran.h (INTEGER_ONLY,REAL_OK): New symbols; fix typo in comment
	(gfc_resolve_iterator): Update prototype
	* array.c (resolve_array_list): Use symbol
	* resolve.c (gfc_resolve_iterator): Rewrite function; use symbols
	* trans-stmt.c (gfc_trans_do): Permit REAL iterators

-- 
Steve

Attachment: doloop2.diff
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]