This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, PR 41112] Do not SRA arrays with non-constant domain bounds
On Thu, 3 Sep 2009, Martin Jambor wrote:
> 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.
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 ;)
Richard.