This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
| Other format: | [Raw text] | |
Vladimir Makarov <vmakarov@redhat.com> writes:Although, I believe that it will work for all cases. And I like it more than my solution.On 10/16/2012 02:44 AM, Richard Sandiford wrote:Vladimir Makarov <vmakarov@redhat.com> writes:On 12-10-15 4:25 PM, Richard Sandiford wrote:Richard Sandiford <rdsandiford@googlemail.com> writes:Vladimir Makarov <vmakarov@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 <vmakarov@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?
You can commit it into the branch. You have to do some work for conflict resolution (I added new helper function in curr_insn_transformation for swapping operands) as the code was changed a lot today.I realise you probably have patches pending as well, so I'm happy to wait until those have gone in and update.
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;
}
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |