RFA: Clarify requirements of process_address

[Richard Sandiford]

Vladimir Makarov <vmakarov@redhat.com> writes:
> On 10/25/2012 05:18 AM, Richard Sandiford wrote:
>> Hi Vlad,
>>
>> When testing other patches, I was misled by:
>>
>>    /* Addresses were legitimate before LRA.  So if the address has
>>       two registers than it can have two of them.  We should also
>>       not worry about scale for the same reason.	 */
>>
>> which I took to mean that process_address only handles pre-LRA addresses.
>> I see now that it actually has to handle addresses created within LRA too,
>> some of which might never have been valid.  That also explains why we have
>> to handle invalid addresses that have no base or index, just a displacement.
>>
>> Here's an attempt to enumerate the cases.  Does it look OK?
>> Tested on x86_64-linux-gnu.
>>
> It is a good start.  We definitely need to have a better understanding 
> GCC addresses therefore good comments are important. It is ok for me to 
> commit.  Thanks for working on this.  You make my life easier.
>
> But  I guess it does not describe all cases, e.g. displacement was valid 
> but after updating offsets for eliminated regs (because stack slots were 
> allocated) it became invalid.

Ah, yeah, thanks.  How about this instead?

Richard


gcc/
	* lra-constraints.c (process_address): Describe the kinds of address
	that we might see.

Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c	2012-10-25 21:07:25.775369947 +0100
+++ gcc/lra-constraints.c	2012-10-25 21:18:30.203368322 +0100
@@ -2496,8 +2496,23 @@ equiv_address_substitution (struct addre
   return change_p;
 }
 
-/* Major function to make reloads for address in operand NOP.  Add to
-   reloads to the list *BEFORE and *AFTER.  We might need to add
+/* Major function to make reloads for an address in operand NOP.
+   The supported cases are:
+
+   1) an address that existed before LRA started, at which point it must
+      have been valid.  These addresses are subject to elimination and
+      may have become invalid due to the elimination offset being out
+      of range.
+
+   2) an address created by forcing a constant to memory (force_const_to_mem).
+      The initial form of these addresses might not be valid, and it is this
+      function's job to make them valid.
+
+   3) a frame address formed from a register and a (possibly zero)
+      constant offset.  As above, these addresses might not be valid
+      and this function must make them so.
+
+   Add reloads to the lists *BEFORE and *AFTER.  We might need to add
    reloads to *AFTER because of inc/dec, {pre, post} modify in the
    address.  Return true for any RTL change.  */
 static bool
@@ -2559,9 +2574,19 @@ process_address (int nop, rtx *before, r
       && process_addr_reg (ad.index_reg_loc, before, NULL, INDEX_REG_CLASS))
     change_p = true;
 
-  /* The address was valid before LRA.  We only change its form if the
-     address has a displacement, so if it has no displacement it must
-     still be valid.  */
+  /* There are three cases where the shape of *ADDR_LOC may now be invalid:
+
+     1) the original address was valid, but either elimination or
+	equiv_address_substitution applied a displacement that made
+	it invalid.
+
+     2) the address is an invalid symbolic address created by
+	force_const_to_mem.
+
+     3) the address is a frame address with an invalid offset.
+
+     All these cases involve a displacement, so there is no point
+     revalidating when there is no displacement.  */
   if (ad.disp_loc == NULL)
     return change_p;
 
@@ -2596,9 +2621,8 @@ process_address (int nop, rtx *before, r
   if (ok_p)
     return change_p;
 
-  /* Addresses were legitimate before LRA.  So if the address has
-     two registers than it can have two of them.  We should also
-     not worry about scale for the same reason.	 */
+  /* Any index existed before LRA started, so we can assume that the
+     presence and shape of the index is valid.  */
   push_to_sequence (*before);
   if (ad.base_reg_loc == NULL)
     {
@@ -2613,7 +2637,7 @@ process_address (int nop, rtx *before, r
 	    rtx insn;
 	    rtx last = get_last_insn ();
 
-	    /* disp => lo_sum (new_base, disp)	*/
+	    /* disp => lo_sum (new_base, disp), case (2) above.  */
 	    insn = emit_insn (gen_rtx_SET
 			      (VOIDmode, new_reg,
 			       gen_rtx_HIGH (Pmode, copy_rtx (*ad.disp_loc))));
@@ -2635,14 +2659,15 @@ process_address (int nop, rtx *before, r
 #endif
 	  if (code < 0)
 	    {
-	      /* disp => new_base  */
+	      /* disp => new_base, case (2) above.  */
 	      lra_emit_move (new_reg, *ad.disp_loc);
 	      *ad.disp_loc = new_reg;
 	    }
 	}
       else
 	{
-	  /* index * scale + disp => new base + index * scale  */
+	  /* index * scale + disp => new base + index * scale,
+	     case (1) above.  */
 	  enum reg_class cl = base_reg_class (mode, as, SCRATCH, SCRATCH);
 
 	  lra_assert (INDEX_REG_CLASS != NO_REGS);
@@ -2656,7 +2681,7 @@ process_address (int nop, rtx *before, r
     }
   else if (ad.index_reg_loc == NULL)
     {
-      /* base + disp => new base  */
+      /* base + disp => new base, cases (1) and (3) above.  */
       /* Another option would be to reload the displacement into an
 	 index register.  However, postreload has code to optimize
 	 address reloads that have the same base and different
@@ -2667,7 +2692,8 @@ process_address (int nop, rtx *before, r
     }
   else
     {
-      /* base + scale * index + disp => new base + scale * index  */
+      /* base + scale * index + disp => new base + scale * index,
+	 case (1) above.  */
       new_reg = base_plus_disp_to_reg (mode, as, &ad);
       *addr_loc = gen_rtx_PLUS (Pmode, new_reg, *ad.index_loc);
     }