On Mon, Aug 18, 2014 at 8:59 AM, Patrick Palka <patr...@parcs.ath.cx> wrote:
> On Mon, Aug 18, 2014 at 8:50 AM, Richard Biener
> <richard.guent...@gmail.com> wrote:
>> On Mon, Aug 18, 2014 at 2:31 PM, Patrick Palka <patr...@parcs.ath.cx> wrote:
>>> On Mon, Aug 18, 2014 at 6:48 AM, Richard Biener
>>> <richard.guent...@gmail.com> wrote:
>>>> On Mon, Aug 18, 2014 at 4:00 AM, Patrick Palka <patr...@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.
I've attached a solution that seems to work. It fixes the ICE and
generates correct code for non-constant stores into register arrays.
If this approach turns out to be robust, I will send a revised patch
tomorrow.
diff --git a/gcc/expr.c b/gcc/expr.c
index 58b87ba..11af448 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -4777,6 +4777,7 @@ expand_assignment (tree to, tree from, bool nontemporal)
tree offset;
int unsignedp;
int volatilep = 0;
+ rtx spilled_reg = NULL_RTX;
tree tem;
push_temp_slots ();
@@ -4828,11 +4829,23 @@ expand_assignment (tree to, tree from, bool nontemporal)
if (!MEM_P (to_rtx))
{
- /* We can get constant negative offsets into arrays with broken
- user code. Translate this to a trap instead of ICEing. */
- gcc_assert (TREE_CODE (offset) == INTEGER_CST);
- expand_builtin_trap ();
- to_rtx = gen_rtx_MEM (BLKmode, const0_rtx);
+ if (TREE_CODE (offset) == INTEGER_CST)
+ {
+ /* We have an invalid constant index into a register array.
+ Emit a trap and move on. */
+ expand_builtin_trap ();
+ to_rtx = gen_rtx_MEM (BLKmode, const0_rtx);
+ }
+ else
+ {
+ /* We have a non-constant index to a register array. Perform
+ the indexed store by spilling the register. */
+ spilled_reg = to_rtx;
+ to_rtx = assign_stack_temp
+ (GET_MODE (to_rtx),
+ GET_MODE_SIZE (GET_MODE (to_rtx)));
+ emit_move_insn (to_rtx, spilled_reg);
+ }
}
offset_rtx = expand_expr (offset, NULL_RTX, VOIDmode, EXPAND_SUM);
@@ -4959,6 +4972,9 @@ expand_assignment (tree to, tree from, bool nontemporal)
get_alias_set (to), nontemporal);
}
+ if (spilled_reg)
+ emit_move_insn (spilled_reg, to_rtx);
+
if (result)
preserve_temp_slots (result);
pop_temp_slots ();
