[PATCH, PR 41112] Do not SRA arrays with non-constant domain bounds

[Martin Jambor]

On Thu, Sep 03, 2009 at 01:52:28PM +0200, Richard Guenther wrote:
> On Thu, 3 Sep 2009, Martin Jambor wrote:
> > On Thu, Sep 03, 2009 at 01:05:11PM +0200, Richard Guenther wrote:
> > > On Thu, Sep 3, 2009 at 12:51 PM, Martin Jambor<mjambor@suse.cz> wrote:
> > > > On Wed, Sep 02, 2009 at 09:06:27PM +0200, Richard Guenther wrote:
> > > >> On Wed, 2 Sep 2009, Martin Jambor wrote:
> > > >> > On Wed, Sep 02, 2009 at 06:18:22PM +0200, Richard Guenther wrote:
> > > >> > > On Wed, 2 Sep 2009, Martin Jambor wrote:
> > > >> > > > the patch below fixes PR 41112 by simply not scalarizing arrays with
> > > >> > > > domain bounds which are not integer constants.
> > > >> > > >
> > > >> > > > I have asked for a testcase addition in the bugzilla since I don't
> > > >> > > > really know where to put it. Â I got freaked out that there is no
> > > >> > > > pr<number> file in gcc/testsuite/gnats.dg and thus have reasons to
> > > >> > > > believe it should go someplace else.
> > > >> > > >
> > > >> > > > I have just bootstrapped and regression tested this on x86_64-linux,
> > > >> > > > obviously including Ada, revision 151322 (i.e. after VTA).
> > > >> > > >
> > > >> > > > OK for trunk?
> > > >> > >
> > > >> > > Hmm. Â I would have expected the array to be disqualified once we
> > > >> > > come along an ARRAY_REF/ARRAY_RANGE_REF with non-constant and non-NULL
> > > >> > > operand 2.
> > > >> >
> > > >> > Yes, get_ref_base_and_extent should do that... the only way I can see
> > > >> > it not happening (I don't really understand the ADA source) is when
> > > >> > the array is one element long, just with an unknown index of that one
> > > >> > element. Â Then the maxsize returned by get_ref_base_and_extent would
> > > >> > equal to the size and SRA would happily try to scalarize it.
> > > >>
> > > >> get_ref_base_and_extent should indeed return the offset of the first
> > > >> array element and max_size of -1. Â So I wonder what is going wrong
> > > >> and rather would fix the real problem than papering over it with
> > > >> the type based check.
> > > >>
> > > >
> > > > Hm, apparently the patch preventing SRA from creating clearly
> > > > unnecessary replacements already hides this bug.
> > > >
> > > > But anyway, since Eric's mail suggested that the array in question is
> > > > three elements long, I'll investigate further to make sure we have no
> > > > bugs. Â However, I still believe that we will have to specifically
> > > > handle (or paper over) the case when the array is one element long,
> > > > there is no way to distinguish that case from the stuff returned by
> > > > get_ref_base_and_extent.
> > > 
> > > But it's ok to scalarize a one-element long array with varying
> > > lower/upper bound.
> > > The only valid access is to that single element, what index value is actually
> > > used is irrelevant - index - min_bound has to be zero always.  The question is
> > > only whether building the replacement reference works ok for this case.
> > > 
> > 
> > Well, the bug is exactly that it doesn't.  The reason is that the
> > function which builds these expressions (build_ref_for_offset_1) has
> > only offset as the input and that cannot be converted to an array
> > index without inserting runtime calculations (which might or might not
> > be optimized out later).  I am not sure whether it's worth it adding
> > such capabilities to the function (messing more with the gimple
> > iterators of the callers).  But it is doable, of course.
> 
> But isn't a single-element array scalarized to a scalar?  So, in the
> end I don't see why it should be that complicated to do ;)
> 

Replacing a reference to such an element with a single scalar is not a
problem, of course.

However, often I need to load the value of that scalar replacement
from the original aggregate (when it is a parameter or the aggregate
is assigned to as a whole) or store its value back to the aggregate
(when it is being used/returned as a whole).  Then I need to create
references into the aggregates from the offset.

One option how to allow this this would be to create a special version
of generate_subtree_copies which would be used to do this (as opposed
to piecemeal loads/stores from a different aggregate) which would
achieve this by unsharing the original expressions rather than
re-creating them from the offsets.  I was contemplating this in spring
but did not proceed as it was already quite late in the review
process.

Martin