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

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

[Martin Jambor]

Hi,

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?

Thanks,

Martin



2009-09-02  Martin Jambor  <mjambor@suse.cz>

	* tree-sra.c (type_internals_preclude_sra_p): Disqualify array
	types with non-constant domain bounds.

Index: mine/gcc/tree-sra.c
===================================================================
--- mine.orig/gcc/tree-sra.c
+++ mine/gcc/tree-sra.c
@@ -548,6 +548,8 @@ type_internals_preclude_sra_p (tree type
       return false;
 
     case ARRAY_TYPE:
+      if (TREE_CODE (TYPE_MIN_VALUE (TYPE_DOMAIN (type))) != INTEGER_CST)
+	return false;
       et = TREE_TYPE (type);
 
       if (AGGREGATE_TYPE_P (et))

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

[Richard Guenther]

On Wed, 2 Sep 2009, Martin Jambor wrote:

> Hi,
> 
> 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.  With the attached patch you no longer scalarize
struct { int i; int array[x..y]; } if only i is ever accessed.

So - why is this check necessary?  Maybe the disqualification is
not done properly?

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 
> 
> 2009-09-02  Martin Jambor  <mjambor@suse.cz>
> 
> 	* tree-sra.c (type_internals_preclude_sra_p): Disqualify array
> 	types with non-constant domain bounds.
> 
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -548,6 +548,8 @@ type_internals_preclude_sra_p (tree type
>        return false;
>  
>      case ARRAY_TYPE:
> +      if (TREE_CODE (TYPE_MIN_VALUE (TYPE_DOMAIN (type))) != INTEGER_CST)
> +	return false;
>        et = TREE_TYPE (type);
>  
>        if (AGGREGATE_TYPE_P (et))
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex

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

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

> the patch below fixes PR 41112 by simply not scalarizing arrays with
> domain bounds which are not integer constants.

Thanks for the quick fix!

> 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.

Please install the attached testcase as gnat.dg/array8.adb.

-- 
Eric Botcazou

[-- Attachment #2: array8.adb --]
[-- Type: text/x-adasrc, Size: 575 bytes --]

-- { dg-do compile }
-- { dg-options "-O2" }

PROCEDURE Array8 IS

  function ID (I : Integer) return Integer is
  begin
    return I;
  end;

  SUBTYPE STB IS INTEGER RANGE ID(-8) .. -5;

  TYPE TB IS ARRAY (STB RANGE <>) OF INTEGER;

  GENERIC
    B1 : TB;
  PROCEDURE PROC1;

  PROCEDURE PROC1 IS
  BEGIN
    IF B1'FIRST /= -8 THEN
      raise Program_Error;
    ELSIF B1'LAST /= ID(-5) THEN
      raise Program_Error;
    ELSIF B1 /= (7, 6, 5, 4) THEN
      raise Program_Error;
    END IF;
  END;

  PROCEDURE PROC2 IS NEW PROC1 ((7, 6, ID(5), 4));

BEGIN
  PROC2;
END;

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

[Martin Jambor]

Hi,
On Wed, Sep 02, 2009 at 06:18:22PM +0200, Richard Guenther wrote:
> On Wed, 2 Sep 2009, Martin Jambor wrote:
> 
> > Hi,
> > 
> > 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.

> With the attached patch you no longer scalarize
> struct { int i; int array[x..y]; } if only i is ever accessed.
> 

You are right.  I simply thought this scenario was rather rare and
thus such a quick fix was acceptable.  On the other hand, tree-sra
does not really traverse the ARRAY_REFs and ARRAY_RANGE_REFs on its
own but rather relies on get_ref_base_and_extent.  And so I would have
to add a special scan of every handled_component_p expression to look
for array types like this and I did not really want to.

Obviously, if constructs like the one above do happen often, I can do
that.  Eric, do you think it is worth it?

I hope this explains it,

Martin


> So - why is this check necessary?  Maybe the disqualification is
> not done properly?
> 
> Thanks,
> Richard.
> 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > 
> > 2009-09-02  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	* tree-sra.c (type_internals_preclude_sra_p): Disqualify array
> > 	types with non-constant domain bounds.
> > 
> > Index: mine/gcc/tree-sra.c
> > ===================================================================
> > --- mine.orig/gcc/tree-sra.c
> > +++ mine/gcc/tree-sra.c
> > @@ -548,6 +548,8 @@ type_internals_preclude_sra_p (tree type
> >        return false;
> >  
> >      case ARRAY_TYPE:
> > +      if (TREE_CODE (TYPE_MIN_VALUE (TYPE_DOMAIN (type))) != INTEGER_CST)
> > +	return false;
> >        et = TREE_TYPE (type);
> >  
> >        if (AGGREGATE_TYPE_P (et))
> > 
> > 

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

[Eric Botcazou]

> 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.

-gnatD generates a .dg file containing the expanded Ada code just before its 
translation to GENERIC.  IIRC the problem was pertaining to one of

      [subtype array8__proc2GP515__T10b is array8__tb (R2b .. R2b +
        3)]

            [subtype array8__proc2__T15b is array8__tb (R2b .. R2b +
              3)]

i.e arrays with fixed size but variable bounds.

> You are right.  I simply thought this scenario was rather rare and
> thus such a quick fix was acceptable.  On the other hand, tree-sra
> does not really traverse the ARRAY_REFs and ARRAY_RANGE_REFs on its
> own but rather relies on get_ref_base_and_extent.  And so I would have
> to add a special scan of every handled_component_p expression to look
> for array types like this and I did not really want to.
>
> Obviously, if constructs like the one above do happen often, I can do
> that.  Eric, do you think it is worth it?

I would say no as far as Ada is concerned.

-- 
Eric Botcazou

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

[Richard Guenther]

On Wed, 2 Sep 2009, Martin Jambor wrote:

> Hi,
> On Wed, Sep 02, 2009 at 06:18:22PM +0200, Richard Guenther wrote:
> > On Wed, 2 Sep 2009, Martin Jambor wrote:
> > 
> > > Hi,
> > > 
> > > 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.

Thanks,
Richard.

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

[Martin Jambor]

Hi,

On Wed, Sep 02, 2009 at 09:06:27PM +0200, Richard Guenther wrote:
> On Wed, 2 Sep 2009, Martin Jambor wrote:
> 
> > Hi,
> > On Wed, Sep 02, 2009 at 06:18:22PM +0200, Richard Guenther wrote:
> > > On Wed, 2 Sep 2009, Martin Jambor wrote:
> > > 
> > > > Hi,
> > > > 
> > > > 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.

Martin

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

[Richard Guenther]

On Thu, Sep 3, 2009 at 12:51 PM, Martin Jambor<mjambor@suse.cz> wrote:
> Hi,
>
> On Wed, Sep 02, 2009 at 09:06:27PM +0200, Richard Guenther wrote:
>> On Wed, 2 Sep 2009, Martin Jambor wrote:
>>
>> > Hi,
>> > On Wed, Sep 02, 2009 at 06:18:22PM +0200, Richard Guenther wrote:
>> > > On Wed, 2 Sep 2009, Martin Jambor wrote:
>> > >
>> > > > Hi,
>> > > >
>> > > > 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.

Richard.

> Martin
>
>

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

[Martin Jambor]

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

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

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3450 bytes --]

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.

Re: [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

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

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4998 bytes --]

On Thu, 3 Sep 2009, Martin Jambor wrote:

> 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.

Ah, I forgot about that...

> 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.

Another workaround (well, maybe it's even a hack ...) would be to
force the lower bound to zero when re-generating the access.  As you
have zero-based offsets you can build an ARRAY_REF with integer_zero_node
as operand 2 - that (hopefully) will work for all places that correctly
look at ARRAY_REFs...

Richard.