Fortran array indexing on 64-bit targets & snapshot 971023

Fortran array indexing on 64-bit targets & snapshot 971023

[Toon Moene]

Hi,

It seems that Richard Henderson's `expr.c' (get_inner_reference)  
patch didn't make into snapshot 971023.

Was there a compelling reason to exclude it ?

Without this change his update to the Fortran frontend has little  
or no effect.

Cheers,
Toon.

Re: Fortran array indexing on 64-bit targets & snapshot 971023

[Jeffrey A Law]

  In message < 9710241223.AA02380@moene.indiv.nluug.nl >you write:
  > Hi,
  > 
  > It seems that Richard Henderson's `expr.c' (get_inner_reference)  
  > patch didn't make into snapshot 971023.
  > 
  > Was there a compelling reason to exclude it ?
Jim had some questions about it, which nobody has answered yet:

  Jim:
  > The get_inner_reference and f/com.c patch are both doing the same thing,
  > forcing array indexing calculations to sizetype.  If we assume that sizetype
  > is an efficient type, then this is the right thing to do.  If sizetype is
  > larger than a word, then this may result in worse code.  That is probably
  > a case we shouldn't worry about.  One would expect that changing the type of
  > the expression might cause it to compute the wrong value, but I can't see
  > any obvious case where it would be different.  This might be a language bias.
  > The low_bound will always be zero for C and one for Fortran.  It can be
  > other things for a more complicated language like Ada.  It is interesting to
  > note that in expand_expr in the ARRAY_REF case, there is similar code, with
  > a FIXME comment that claims it isn't right.  If this comment is true, then
  > these changes may cause a problem.  If there is a problem, we can probably
  > exclude the failing cases, and still support the Fortran lower_bound = 1
  > case which should always be OK.
The comment about ARRAY_REF in expr.c in particular worries me that we
might be missing some important issue.

I believe Jim is referring to this:

    case ARRAY_REF:
      if (TREE_CODE (TREE_TYPE (TREE_OPERAND (exp, 0))) != ARRAY_TYPE)
        abort ();

      {
        tree array = TREE_OPERAND (exp, 0);
        tree domain = TYPE_DOMAIN (TREE_TYPE (array));
        tree low_bound = domain ? TYPE_MIN_VALUE (domain) : integer_zero_node;
        tree index = TREE_OPERAND (exp, 1);
        tree index_type = TREE_TYPE (index);
        HOST_WIDE_INT i;

        /* Optimize the special-case of a zero lower bound.

           We convert the low_bound to sizetype to avoid some problems
           with constant folding.  (E.g. suppose the lower bound is 1,
           and its mode is QI.  Without the conversion,  (ARRAY
           +(INDEX-(unsigned char)1)) becomes ((ARRAY+(-(unsigned char)1))
           +INDEX), which becomes (ARRAY+255+INDEX).  Oops!)

           But sizetype isn't quite right either (especially if
           the lowbound is negative).  FIXME */

        if (! integer_zerop (low_bound))
          index = fold (build (MINUS_EXPR, index_type, index,
                               convert (sizetype, low_bound)));

Re: Fortran array indexing on 64-bit targets & snapshot 971023

[Toon Moene]

  In message < 9710241223.AA02380@moene.indiv.nluug.nl > I wrote:

  > It seems that Richard Henderson's `expr.c' (get_inner_reference)  
  > patch didn't make into snapshot 971023.
  >
  > Was there a compelling reason to exclude it ?

Jeff:

> Jim had some questions about it, which nobody has answered yet:

[ Complicated discussion about the special handling of zero lower  
bound array references elided ]

Yes, I've looked into this and didn't understand it either.

Nevertheless, to inject some numbers into this discussion (using  
Patick Queutey's CFD benchmark example on a 500 Mhz Alpha):

egcs-971023, bare (3 runs):

7.60user 0.02system 0:20.87elapsed 36%CPU (0avgtext+0avgdata  
0maxresident)k
7.25user 0.01system 0:11.27elapsed 64%CPU (0avgtext+0avgdata  
0maxresident)k
7.78user 0.02system 0:11.18elapsed 69%CPU (0avgtext+0avgdata  
0maxresident)k

After applying Richard Henderson's `exp.c' patch (4 runs):

4.51user 0.14system 0:09.17elapsed 50%CPU (0avgtext+0avgdata  
0maxresident)k
4.53user 0.01system 0:08.20elapsed 55%CPU (0avgtext+0avgdata  
0maxresident)k
4.15user 0.02system 0:07.72elapsed 54%CPU (0avgtext+0avgdata  
0maxresident)k
4.37user 0.01system 0:07.29elapsed 60%CPU (0avgtext+0avgdata  
0maxresident)k

(with your `use' patch I get between 3.6 and 3.9 seconds).

So I hope a decision will be reached (to say the least :-)

Cheers,
Toon.

Re: Fortran array indexing on 64-bit targets & snapshot 971023

[Toon Moene]

> I believe Jim is referring to this:

[ In expand_expr, case ARRAY_REF: ]

[ ... ]

        /* Optimize the special-case of a zero lower bound.

           We convert the low_bound to sizetype to avoid some problems
           with constant folding.  (E.g. suppose the lower bound is 1,
           and its mode is QI.  Without the conversion,  (ARRAY
           +(INDEX-(unsigned char)1)) becomes ((ARRAY+(-(unsigned  
char)1))
           +INDEX), which becomes (ARRAY+255+INDEX).  Oops!)

           But sizetype isn't quite right either (especially if
           the lowbound is negative).  FIXME */

[ ... ]

The problem is that you cannot be sure that sizetype is signed (see  
functions `make_signed_type' and `make_unsigned_type' in  
stor-layout.c).

Why it's done this way is beyond me ...

HTH,
Toon.

Re: Fortran array indexing on 64-bit targets & snapshot 971023

[Jeffrey A Law]

  In message < 9710261322.AA08510@moene.indiv.nluug.nl >you write:
  > > I believe Jim is referring to this:
  > 
  > [ In expand_expr, case ARRAY_REF: ]
  > 
  > [ ... ]
  > 
  >         /* Optimize the special-case of a zero lower bound.
  > 
  >            We convert the low_bound to sizetype to avoid some problems
  >            with constant folding.  (E.g. suppose the lower bound is 1,
  >            and its mode is QI.  Without the conversion,  (ARRAY
  >            +(INDEX-(unsigned char)1)) becomes ((ARRAY+(-(unsigned  
  > char)1))
  >            +INDEX), which becomes (ARRAY+255+INDEX).  Oops!)
  > 
  >            But sizetype isn't quite right either (especially if
  >            the lowbound is negative).  FIXME */
  > 
  > [ ... ]
  > 
  > The problem is that you cannot be sure that sizetype is signed (see  
  > functions `make_signed_type' and `make_unsigned_type' in  
  > stor-layout.c).
  > 
  > Why it's done this way is beyond me ...
I'd been thinking about this offline and was starting to wonder if
the FIXME really meant that the problem was sizetype was the wrong
type for the conversion and could cause problems.

And the more I think about it, the more I believe that's the real
issue with that code.

For historical reasons, the call into make_[un]signed_type has always
been with the argument "INT_TYPE_SIZE".  And if no value for sizetype
existed, then it was set to the type created by that first call.

[ An aside, I think all the front ends actually assign a real sizetype
  independent of this braindamage in make_[un]signed_type. ]

So, consider what happens with sizeof (int) < sizeof (pointer) and
the lowbound is negative.  We run into the exact same problem
mentioned in the comment.

So it would seem like the comment really means that "sizetype" may
not be the correct type for the conversion.  But that's the closest
global thing we've currently got to the correct type -- though I wonder
why indextype wasn't used.  Oh I see, we convert index to sizetype
just after we fold out the bias.

It's worth noting that having sizetype be smaller than the size
of a pointer is a bad thing -- a few obscure ports still do this
(z8 perhaps?), but I'm not too worried about them since they've
already got some serious problems.

I don't really see any reason to not install this patch.

Comments?

jeff

Re: Fortran array indexing on 64-bit targets & snapshot 971023

[Per Bothner]

While hacking on Chill, I got convinced that the way Gcc represents
ranges just does not work.

Gcc represents a range as [min_index, max_index].  What type should these
have?  It seems that min_index and max_index should have the same type as
the index type.  This matters for languages like Pascal, Chill, and Ada (?)
that have index types that can be enums.  The problem is that this does
not work for empty ranges.  In that case max_index==min_index-1, which
might well not be in the index type.  This is a problem even for C,
if the index type is unsigned:  While ANIS C does not have empty arrays,
Gcc C does.  The result is a max_index of (size_t)(-1).  Ooops.

On the other hand, using a signed size_t (internally), may cause some
problems with really large ranges.  E.g.:  [-2147483648 .. 2147483647].

Finally, converting a length to an upperbound requires a calcaulation.
If the length is non-constant, converting the upperbound back again
to a length may not constant fold back to the original length, leading
to bad code.

My conclusion is that we should use differt types for the upper bound
and the length of an array (or integer range in general).  The upper
bound should got be stored explicitly;  instead we should store the
length - as an unsigned.  That is we shold get rid of TYPE_MAX_VALUE,
and replace it by TYPE_LENGTH.

But that is too much work, so it will never happen.

	--Per Bothner
Cygnus Solutions     bothner@cygnus.com     http://www.cygnus.com/~bothner

	

Re: Fortran array indexing on 64-bit targets & snapshot 971023

[Jim Wilson]

	I'd been thinking about this offline and was starting to wonder if
	the FIXME really meant that the problem was sizetype was the wrong
	type for the conversion and could cause problems.

I think it is an Ada specific problem.  As Per Bothner mentioned, Ada
can have arbitrary range types.  I suspect the problem occurs only with
Ada, and only if you have certain kinds of range types with certain values
for the lower/upper bound of the array.

However, I am not convinced that the problem actually does exist,
as I can't see any way to actually trigger the problem.  I see no problem
with going ahead with the change, and if a problem does occur, we will then
have a better idea of what the real problem is and how to fix it.

Jim