[PATCH] Fix promotion of const local arrays to static storage

[Richard Biener]

On Mon, Aug 18, 2014 at 2:31 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Mon, Aug 18, 2014 at 6:48 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Mon, Aug 18, 2014 at 4:00 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>>> Hi,
>>>
>>> The fix for PR38615 indirectly broke the promotion of const local arrays
>>> to static storage in many cases.  The commit in question, r143570, made
>>> it so that only arrays that don't potentially escape from the scope in
>>> which they're defined (i.e. arrays for which TREE_ADDRESSABLE is 0) are
>>> candidates for the promotion to static storage.
>>>
>>> The problem is that both the C and C++ frontends contain ancient code
>>> (dating back to 1994) that sets the TREE_ADDRESSABLE bit on arrays
>>> indexed by non-constant or out-of-range indices.  As a result, const
>>> arrays that are indexed by non-constant or out-of-range values are no
>>> longer candidates for promotion to static storage following the fix to
>>> PR38615, because their TREE_ADDRESSABLE bit will always be set.
>>> Consequently, array promotion is essentially broken for a large class of
>>> C and C++ code.  E.g. this simple example is no longer a candidate for
>>> promotion:
>>>
>>>     int
>>>     foo (int i)
>>>     {
>>>       const int x[] = { 1, 2, 3 };
>>>       return x[i]; /* non-constant index */
>>>     }
>>>
>>> This patch removes the ancient code that is responsible for dubiously
>>> setting the TREE_ADDRESSABLE bit on arrays indexed by non-constant or
>>> out-of-range values.  I don't think that this code is necessary or
>>> useful anymore.  Bootstrapped and regtested on x86_64-unknown-linux-gnu,
>>> OK for trunk?
>>
>> This looks good to me - indeed TREE_ADDRESSABLE on variable-indexed
>> things isn't necessary (and before that it was made sure to re-set it
>> before RTL expansion which required it, as update-address-taken would
>> have happily removed TREE_ADDRESSABLE anyway).
>
> Thanks Richard.  Unfortunately I lied when I optimistically said that
> regression testing passed.  This patch makes causes a single new
> regression in gcc.target/i386/pr14289-1.c.  The file looks like:
>
> /* { dg-options "-O0" } */
>
> register int n[2] asm ("ebx");
>
> void
> foo (void)
> {
>   int i = 0;
>   a[i] = 0; /* { dg-error "address of" } */
> }
>
> Whereas previously the C FE would have reported an error for trying to
> set the TREE_ADDRESSABLE bit on a register variable, now an ICE is
> triggered during RTL expansion for trying to index into a register
> array with a non-constant index.  So I am thinking of explicitly
> checking for this scenario and emitting an error.  Do you think such a
> check should be done in the frontends or during RTL expansion?  If
> done during RTL expansion, the optimizers could have a chance of
> turning the non-constant index into a constant one like in this
> example.

I think emitting the error in the above case is to avoid ICEs for
strange cases.  We _should_ be able to expand
a[i] = 0 even with variable index - if only by spilling 'ebx' to memory,
changing it as an array and then loading it back to 'ebx'.

Thus, fixing the ICE would be more welcome here.  Other than that,
yes, erroring in the frontend for non-constant indexed register
variables is ok with me as well.  The above also errors for -O1, right?

Richard.