diff mbox

RFA: Clarify requirements of process_address

Message ID 87ip9ync4y.fsf@talisman.home
State New
Headers show

Commit Message

Richard Sandiford Oct. 25, 2012, 9:18 a.m. UTC
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.

Richard


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

Comments

Vladimir Makarov Oct. 25, 2012, 7:41 p.m. UTC | #1
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.

>
>
> 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 10:10:49.586281642 +0100
> +++ gcc/lra-constraints.c	2012-10-25 10:16:51.025280757 +0100
> @@ -2496,8 +2496,21 @@ 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.  These addresses
> +      must already be valid.
I would add in non-strict sense.
>
diff mbox

Patch

Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c	2012-10-25 10:10:49.586281642 +0100
+++ gcc/lra-constraints.c	2012-10-25 10:16:51.025280757 +0100
@@ -2496,8 +2496,21 @@  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.  These addresses
+      must already be valid.
+
+   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 +2572,18 @@  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 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 +2618,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 +2634,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 +2656,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 +2678,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 +2689,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);
     }