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.
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);
}
