On 10/25/2012 05:45 AM, Richard Sandiford wrote:
Hi Vlad,
As discussed in the reviews, one of the things that worried me was the
combination of:
1) the displacement fixup code in process_address assumes that the address
is exactly equal to BASE_LOC + INDEX_LOC + DISP (with null values
being equivalent to 0).
2) extract_address_regs allows (and has to allow) much more general forms
than that.
3) equiv_address_substitution folds base and index displacements
without checking the shape of the address.
So the code from (1) could end up fixing a displacement created by (3)
even if the address isn't a simple BASE_LOC + INDEX_LOC + DISP.
There's a similar problem with the relationship between INDEX_LOC
and INDEX_REG_LOC: we assume the scale is 1 unless INDEX is a MULT
of the index register.
This patch adds two utility functions, one to test whether the address
is simple enough for the fixup code to handle, and one to see whether
the index scale is known. The latter handles ASHIFT as well as MULT,
since ASHIFT can be used in out-of-MEM address operands.
That is really nice.
The condition:
/* If the address already has a displacement, we can simply add the
new displacement to it. */
if (ad->disp_loc)
return true;
might leave a bit of a hole, but the asserts you added to
extract_address_regs should mean that disp_loc really is a
displacement (thanks for those btw).
I am not sure about it. UNSPEC is also treated as a displacement. I
don't think it can be combined.
In principle, we could apply a base or index displacement even in cases
that process_address can't fix up, test whether the result is valid,
and revert to the normal behaviour of reloading the full base or index
value if not. That's not going to be useful for x86 (or for MIPS :-))
and I'm not planning to do it, but the valid_address_p patch I just sent
ought to help.
Tested on x86_64-linux-gnu. Also tested by making sure that there
were no changes in assembly output for a set of gcc .ii files
(i.e. these extra checks didn't affect the code on x86_64 at least).
OK to install?
Probably we should check UNSPEC too. Otherwise, it looks ok. Thanks,
Richard.
I see a potential bug here. We should not reject new equiv values for
base and index here. After we decided to use equiv it should be changed
everywhere as we remove init insns.
So if we have base + index and each of these is changed by pair of
pseudos (I think it is an extremely rare situation) we now have 4
pseudos which we should use. Another situation is pseudo + unspec and
we change pseudo by reg+offset. We should deal with this somehow.
I guess, we should generate reloads in equiv_address_substitution for
such rare cases.
Richard
gcc/
* lra-constraints.c (get_index_scale, can_add_disp_p): New functions.
(equiv_address_substitution): Use them.
Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c 2012-10-25 09:50:13.005284668 +0100
+++ gcc/lra-constraints.c 2012-10-25 09:55:17.376283922 +0100
@@ -756,6 +756,28 @@ extract_address_regs (enum machine_mode
ad->index_loc = ad->index_reg_loc;
}
+/* Return the scale applied to *AD->INDEX_REG_LOC, or 0 if the index is
+ more complicated than that. */
+static HOST_WIDE_INT
+get_index_scale (struct address *ad)
+{
+ rtx index = *ad->index_loc;
+ if (GET_CODE (index) == MULT
+ && CONST_INT_P (XEXP (index, 1))
+ && ad->index_reg_loc == &XEXP (index, 0))
+ return INTVAL (XEXP (index, 1));
+
+ if (GET_CODE (index) == ASHIFT
+ && CONST_INT_P (XEXP (index, 1))
+ && ad->index_reg_loc == &XEXP (index, 0))
+ return (HOST_WIDE_INT) 1 << INTVAL (XEXP (index, 1));
+
+ if (ad->index_reg_loc == ad->index_loc)
+ return 1;
+
+ return 0;
+}
+
/* The page contains major code to choose the current insn alternative
@@ -2430,6 +2452,40 @@ base_plus_disp_to_reg (enum machine_mode
return new_reg;
}
+/* Return true if we can add a displacement to address ADDR_LOC,
+ which is described by AD, even if that makes the address invalid.
+ The fix-up code requires any new address to be the sum of the base,
+ index and displacement fields of an AD-like structure. */
+static bool
+can_add_disp_p (struct address *ad, rtx *addr_loc)
+{
+ /* Automodified addresses have a fixed form. */
+ if (ad->base_modify_p)
+ return false;
+
+ /* If the address already has a displacement, we can simply add the
+ new displacement to it. */
+ if (ad->disp_loc)
+ return true;
+
+ /* If the address is entirely a base or index, we can try adding
+ a constant to it. */
+ if (addr_loc == ad->base_reg_loc || addr_loc == ad->index_loc)
+ return true;
+
+ /* Likewise if the address is entirely a sum of the base and index. */
+ if (GET_CODE (*addr_loc) == PLUS)
+ {
+ rtx *op0 = &XEXP (*addr_loc, 0);
+ rtx *op1 = &XEXP (*addr_loc, 1);
+ if (op0 == ad->base_reg_loc && op1 == ad->index_loc)
+ return true;
+ if (op1 == ad->base_reg_loc && op0 == ad->index_loc)
+ return true;
+ }
+ return false;
+}
+
/* Make substitution in address AD in space AS with location ADDR_LOC.
Update AD and ADDR_LOC if it is necessary. Return true if a
substitution was made. */
@@ -2475,7 +2531,8 @@ equiv_address_substitution (struct addre
}
else if (GET_CODE (new_base_reg) == PLUS
&& REG_P (XEXP (new_base_reg, 0))
- && CONST_INT_P (XEXP (new_base_reg, 1)))
+ && CONST_INT_P (XEXP (new_base_reg, 1))
+ && can_add_disp_p (ad, addr_loc))
{
disp += INTVAL (XEXP (new_base_reg, 1));
*ad->base_reg_loc = XEXP (new_base_reg, 0);
@@ -2484,12 +2541,6 @@ equiv_address_substitution (struct addre
if (ad->base_reg_loc2 != NULL)
*ad->base_reg_loc2 = *ad->base_reg_loc;
}
- scale = 1;
- if (ad->index_loc != NULL && GET_CODE (*ad->index_loc) == MULT)
- {
- lra_assert (CONST_INT_P (XEXP (*ad->index_loc, 1)));
- scale = INTVAL (XEXP (*ad->index_loc, 1));
- }
if (index_reg != new_index_reg)
{
if (REG_P (new_index_reg))
@@ -2499,7 +2550,9 @@ equiv_address_substitution (struct addre
}
else if (GET_CODE (new_index_reg) == PLUS
&& REG_P (XEXP (new_index_reg, 0))
- && CONST_INT_P (XEXP (new_index_reg, 1)))
+ && CONST_INT_P (XEXP (new_index_reg, 1))
+ && can_add_disp_p (ad, addr_loc)
+ && (scale = get_index_scale (ad)))
{
disp += INTVAL (XEXP (new_index_reg, 1)) * scale;
*ad->index_reg_loc = XEXP (new_index_reg, 0);
