Vladimir Makarov <vmaka...@redhat.com> writes:
> On 10/16/2012 02:44 AM, Richard Sandiford wrote:
>> Vladimir Makarov <vmaka...@redhat.com> writes:
>>> On 12-10-15 4:25 PM, Richard Sandiford wrote:
>>>> Richard Sandiford <rdsandif...@googlemail.com> writes:
>>>>> Vladimir Makarov <vmaka...@redhat.com> writes:
>>>>>> After committing a patch yesterday to implement proposals from a
>>>>>> review, I found that GCC crashes on SPEC2000 gap. LRA is trying to find
>>>>>> a mode of operand (const_int 1) in *lea_general_1 insn and can not find
>>>>>> it as the operand and insn template operand has VOIDmode.
>>>>>>
>>>>>> There are still cases when context lookup is necessary to find a mode of
>>>>>> the operand. So I am reversing the change I did yesterday.
>>>>>>
>>>>>> The patch is committed as rev. 192462.
>>>>>>
>>>>>> 2012-10-15 Vladimir Makarov <vmaka...@redhat.com>
>>>>>>
>>>>>> * lra-int.h (lra_get_mode): Remove.
>>>>>> * lra-constraints.c (find_mode, get_op_mode): New functions.
>>>>>> (match_reload): Use get_op_mode instead of lra_get_mode.
>>>>>> (process_alt_operands, curr_insn_transform): Ditto.
>>>>> But my objection to this code still stands. It's wrong to assume
>>>>> that an operand to an rtx has the same mode as the containing rtx.
>>>>>
>>>>> Please add a testcase that shows the problem.
>>>> (...because I was hoping to have a look myself). But if that's too
>>>> difficult to reduce, then which operand to *lea_general_1 was the problem?
>>>> The pattern looks like:
>>>>
>>>> (define_insn_and_split "*lea_general_1"
>>>> [(set (match_operand 0 "register_operand" "=r")
>>>> (plus (plus (match_operand 1 "index_register_operand" "l")
>>>> (match_operand 2 "register_operand" "r"))
>>>> (match_operand 3 "immediate_operand" "i")))]
>>>>
>>>> So operands 0, 1 and 2 should have been registers. Operand 3 never
>>>> needs reloading, so its mode shouldn't matter.
>>>>
>>> In this case the const needs a reload as it was a pseudo substituted by
>>> equiv constant.
>> But in that case the operand modes we needed were there in the original
>> instruction operands. If equiv substitution is causing us to lose track
>> of those operand modes, then that needs to be fixed.
>>
>> If the pattern had instead been:
>>
>> (set (match_operand:SI 0 "register_operand" "=r")
>> (unspec:SI [(match_operand 1 "register_operand" "r")] UNSPEC_FOO))
>>
>> and equiv substitution replaced operand 1 with a const_int without
>> the original mode being recorded anywhere, then we'd have no way
>> of recovering that mode from the pattern itself, because the modes
>> of unspec parameters are entirely target-specific.
>>
> I remember I thought about this. The case is rare, it is better to
> calculate than keep mode for each operand of each insn. LRA speed is
> achieved by minimizations of keeping data for each operand. We keep
> only locations. I believe it is a better approach.
I don't think we need to keep a _global_ tab on the operand modes. The nice
thing about representing intermediate steps in rtl is that the substituted,
unreloaded operands only appear temporarily during curr_insn_transform.
Once that function has finished reloading the instruction, the instruction's
operands have the right form again.
This is the kind of thing I had in mind. Tested on x86_64-linux-gnu
({,-m32}), no regressions. Does it look OK? And does it work with
the SPEC testcase?
I realise you probably have patches pending as well, so I'm happy to
wait until those have gone in and update.
Richard
gcc/
* lra-int.h (lra_operand_data): Add is_address field.
* lra.c (debug_operand_data): Initialize is_address field.
(get_static_insn_data): Likewise.
(setup_operand_alternative): Record is_address in operand data.
(lra_set_insn_recog_data): Initialize is_address field.
* lra-constraints.c (curr_operand_mode): New array.
(init_curr_operand_mode): New function.
(find_mode, get_op_mode): Delete.
(match_reload): Use curr_operand_mode rather than get_op_mode.
(process_alt_operands): Likewise. Remove VOIDmode check from
CONST_OK_POOL_P check.
(curr_insn_transform): Likewise. Swap the operand modes when
swapping the operands.
(lra_constraints): Call init_curr_operand_mode.
Index: gcc/lra-int.h
===================================================================
--- gcc/lra-int.h 2012-10-16 19:32:48.000000000 +0100
+++ gcc/lra-int.h 2012-10-16 20:35:53.272728031 +0100
@@ -157,6 +157,8 @@ struct lra_operand_data
This field is set up every time when corresponding
operand_alternative in lra_static_insn_data is set up. */
unsigned int early_clobber : 1;
+ /* True if the operand is an address. */
+ unsigned int is_address : 1;
};
/* Info about register in an insn. */
Index: gcc/lra.c
===================================================================
--- gcc/lra.c 2012-10-16 19:32:48.000000000 +0100
+++ gcc/lra.c 2012-10-16 20:36:19.042726596 +0100
@@ -518,7 +518,7 @@ static struct lra_operand_data debug_ope
NULL, /* alternative */
VOIDmode, /* We are not interesting in the operand mode. */
OP_IN,
- 0, 0, 0
+ 0, 0, 0, 0
};
/* The following data are used as static insn data for all debug
@@ -619,6 +619,7 @@ get_static_insn_data (int icode, int nop
= (data->operand[i].constraint[0] == '=' ? OP_OUT
: data->operand[i].constraint[0] == '+' ? OP_INOUT
: OP_IN);
+ data->operand[i].is_address = false;
}
for (i = 0; i < ndup; i++)
data->dup_num[i] = recog_data.dup_num[i];
@@ -824,6 +825,7 @@ setup_operand_alternative (lra_insn_reco
break;
case 'p':
+ static_data->operand[i].is_address = true;
op_alt->is_address = 1;
op_alt->cl = (reg_class_subunion[(int) op_alt->cl]
[(int) base_reg_class (VOIDmode,
@@ -845,6 +847,7 @@ setup_operand_alternative (lra_insn_reco
}
if (EXTRA_ADDRESS_CONSTRAINT (c, p))
{
+ static_data->operand[i].is_address = true;
op_alt->is_address = 1;
op_alt->cl
= (reg_class_subunion
@@ -1063,6 +1066,7 @@ lra_set_insn_recog_data (rtx insn)
insn_static_data->operand[i].constraint = constraints[i];
insn_static_data->operand[i].strict_low = false;
insn_static_data->operand[i].is_operator = false;
+ insn_static_data->operand[i].is_address = false;
}
}
for (i = 0; i < insn_static_data->n_operands; i++)
Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c 2012-10-16 19:32:48.000000000 +0100
+++ gcc/lra-constraints.c 2012-10-16 20:49:42.717681827 +0100
@@ -138,11 +138,13 @@
static int bb_reload_num;
/* The current insn being processed and corresponding its data (basic
- block, the insn data, and the insn static data. */
+ block, the insn data, the insn static data, and the mode of each
+ operand). */
static rtx curr_insn;
static basic_block curr_bb;
static lra_insn_recog_data_t curr_id;
static struct lra_static_insn_data *curr_static_id;
+static enum machine_mode curr_operand_mode[MAX_RECOG_OPERANDS];
@@ -298,6 +300,27 @@ get_equiv_substitution (rtx x)
gcc_unreachable ();
}
+/* Set up curr_operand_mode. */
+static void
+init_curr_operand_mode (void)
+{
+ int nop = curr_static_id->n_operands;
+ for (int i = 0; i < nop; i++)
+ {
+ enum machine_mode mode = GET_MODE (*curr_id->operand_loc[i]);
+ if (mode == VOIDmode)
+ {
+ /* The .md mode for address operands is the mode of the
+ addressed value rather than the mode of the address itself. */
+ if (curr_id->icode >= 0 && curr_static_id->operand[i].is_address)
+ mode = Pmode;
+ else
+ mode = curr_static_id->operand[i].mode;
+ }
+ curr_operand_mode[i] = mode;
+ }
+}
+
/* The page contains code to reuse input reloads. */
@@ -876,65 +899,6 @@ #define SMALL_REGISTER_CLASS_P(C)
\
(reg_class_size [(C)] == 1 \
|| (reg_class_size [(C)] >= 1 && targetm.class_likely_spilled_p (C)))
-/* Return mode of WHAT inside of WHERE whose mode of the context is
- OUTER_MODE. If WHERE does not contain WHAT, return VOIDmode. */
-static enum machine_mode
-find_mode (rtx *where, enum machine_mode outer_mode, rtx *what)
-{
- int i, j;
- enum machine_mode mode;
- rtx x;
- const char *fmt;
- enum rtx_code code;
-
- if (where == what)
- return outer_mode;
- if (*where == NULL_RTX)
- return VOIDmode;
- x = *where;
- code = GET_CODE (x);
- outer_mode = GET_MODE (x);
- fmt = GET_RTX_FORMAT (code);
- for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
- {
- if (fmt[i] == 'e')
- {
- if ((mode = find_mode (&XEXP (x, i), outer_mode, what)) != VOIDmode)
- return mode;
- }
- else if (fmt[i] == 'E')
- {
- for (j = XVECLEN (x, i) - 1; j >= 0; j--)
- if ((mode = find_mode (&XVECEXP (x, i, j), outer_mode, what))
- != VOIDmode)
- return mode;
- }
- }
- return VOIDmode;
-}
-
-/* Return mode for operand NOP of the current insn. */
-static inline enum machine_mode
-get_op_mode (int nop)
-{
- rtx *loc;
- enum machine_mode mode;
- bool md_first_p = asm_noperands (PATTERN (curr_insn)) < 0;
-
- /* Take mode from the machine description first. */
- if (md_first_p && (mode = curr_static_id->operand[nop].mode) != VOIDmode)
- return mode;
- loc = curr_id->operand_loc[nop];
- /* Take mode from the operand second. */
- mode = GET_MODE (*loc);
- if (mode != VOIDmode)
- return mode;
- if (! md_first_p && (mode = curr_static_id->operand[nop].mode) != VOIDmode)
- return mode;
- /* Here is a very rare case. Take mode from the context. */
- return find_mode (&PATTERN (curr_insn), VOIDmode, loc);
-}
-
/* If REG is a reload pseudo, try to make its class satisfying CL. */
static void
narrow_reload_pseudo_class (rtx reg, enum reg_class cl)
@@ -969,8 +933,8 @@ match_reload (signed char out, signed ch
rtx in_rtx = *curr_id->operand_loc[ins[0]];
rtx out_rtx = *curr_id->operand_loc[out];
- outmode = get_op_mode (out);
- inmode = get_op_mode (ins[0]);
+ outmode = curr_operand_mode[out];
+ inmode = curr_operand_mode[ins[0]];
if (inmode != outmode)
{
if (GET_MODE_SIZE (inmode) > GET_MODE_SIZE (outmode))
@@ -1698,7 +1662,7 @@ process_alt_operands (int only_alternati
}
op = no_subreg_reg_operand[nop];
- mode = get_op_mode (nop);
+ mode = curr_operand_mode[nop];
win = did_match = winreg = offmemok = constmemok = false;
badop = true;
@@ -2170,8 +2134,7 @@ process_alt_operands (int only_alternati
if (CONST_POOL_OK_P (mode, op)
&& ((targetm.preferred_reload_class
(op, this_alternative) == NO_REGS)
- || no_input_reloads_p)
- && get_op_mode (nop) != VOIDmode)
+ || no_input_reloads_p))
{
const_to_mem = 1;
if (! no_regs_p)
@@ -2976,6 +2939,9 @@ curr_insn_transform (void)
curr_swapped = !curr_swapped;
if (curr_swapped)
{
+ enum machine_mode mode = curr_operand_mode[commutative];
+ curr_operand_mode[commutative] = curr_operand_mode[commutative + 1];
+ curr_operand_mode[commutative + 1] = mode;
x = *curr_id->operand_loc[commutative];
*curr_id->operand_loc[commutative]
= *curr_id->operand_loc[commutative + 1];
@@ -2987,6 +2953,9 @@ curr_insn_transform (void)
}
else
{
+ enum machine_mode mode = curr_operand_mode[commutative];
+ curr_operand_mode[commutative] = curr_operand_mode[commutative + 1];
+ curr_operand_mode[commutative + 1] = mode;
x = *curr_id->operand_loc[commutative];
*curr_id->operand_loc[commutative]
= *curr_id->operand_loc[commutative + 1];
@@ -3025,6 +2994,9 @@ curr_insn_transform (void)
fprintf (lra_dump_file, " Commutative operand exchange in insn %u\n",
INSN_UID (curr_insn));
+ enum machine_mode mode = curr_operand_mode[commutative];
+ curr_operand_mode[commutative] = curr_operand_mode[commutative + 1];
+ curr_operand_mode[commutative + 1] = mode;
tem = *curr_id->operand_loc[commutative];
*curr_id->operand_loc[commutative]
= *curr_id->operand_loc[commutative + 1];
@@ -3156,7 +3128,7 @@ curr_insn_transform (void)
rtx op = *curr_id->operand_loc[i];
rtx subreg = NULL_RTX;
rtx plus = NULL_RTX;
- enum machine_mode mode = get_op_mode (i);
+ enum machine_mode mode = curr_operand_mode[i];
if (GET_CODE (op) == SUBREG)
{
@@ -3174,8 +3146,7 @@ curr_insn_transform (void)
if (CONST_POOL_OK_P (mode, op)
&& ((targetm.preferred_reload_class
(op, (enum reg_class) goal_alt[i]) == NO_REGS)
- || no_input_reloads_p)
- && mode != VOIDmode)
+ || no_input_reloads_p))
{
rtx tem = force_const_mem (mode, op);
@@ -3270,7 +3241,7 @@ curr_insn_transform (void)
enum op_type type = curr_static_id->operand[i].type;
loc = curr_id->operand_loc[i];
- mode = get_op_mode (i);
+ mode = curr_operand_mode[i];
if (GET_CODE (*loc) == SUBREG)
{
reg = SUBREG_REG (*loc);
@@ -3730,6 +3701,7 @@ lra_constraints (bool first_p)
curr_id = lra_get_insn_recog_data (curr_insn);
curr_static_id = curr_id->insn_static_data;
init_curr_insn_input_reloads ();
+ init_curr_operand_mode ();
if (curr_insn_transform ())
changed_p = true;
}
