diff mbox

Fix promotion of const local arrays to static storage

Message ID CA+C-WL960g+JJY99HMsVofMniUogW1uoAzDLye7JsBK=ddctkA@mail.gmail.com
State New
Headers show

Commit Message

Patrick Palka Aug. 18, 2014, 2:28 p.m. UTC 
On Mon, Aug 18, 2014 at 8:59 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> 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.

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 mbox

Patch

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 ();