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

[Patrick Palka <patrick at parcs dot ath dot cx>]

On Mon, Aug 18, 2014 at 8:50 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> 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'.

Good point.

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

With the patch, the above doesn't error for -O1 because a[i] is
trivially optimized to a[0] -- a constant index -- before RTL
expansion.  I'll take a stab at fixing the ICE, then.