| Message ID | 87pq4ijeoh.fsf@talisman.home |
|---|---|
| State | New |
| Headers | show |
On 12-10-16 5:21 PM, Richard Sandiford wrote: > Vladimir Makarov <vmakarov@redhat.com> writes: >> 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? Although, I believe that it will work for all cases. And I like it more than my solution. So the patch is ok. Thanks for working on this, Richard. > I realise you probably have patches pending as well, so I'm happy to > wait until those have gone in and update. > 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. > > > 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: 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; }