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

[Martin Jambor <mjambor at suse dot cz>]

Hi,

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.

Martin