Vladimir Makarov <vmaka...@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);
}
