| Message ID | 507C4A38.3000005@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. Richard
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. Richard
On 12-10-15 3:10 PM, Richard Sandiford wrote: > 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. I use it as only the last chance to find a mode for a constant. > Please add a testcase that shows the problem. > > It is a SPEC2000 test file. I don't think the license permits it. In insn: (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")))] one operand is (const_int 1). Its mode is VOIDmode. recog_data mode is VOIDmode. The patch takes right mode from PLUS containing it.
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.
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. Richard
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.
On 10/16/2012 04:30 AM, Vladimir Makarov wrote: > In insn: > > (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")))] > > one operand is (const_int 1). Its mode is VOIDmode. recog_data mode is > VOIDmode. The patch takes right mode from PLUS containing it. Shouldn't this use a mode_iterator to put the right mode on the operand? Bernd
Bernd Schmidt <bernds@codesourcery.com> writes: > On 10/16/2012 04:30 AM, Vladimir Makarov wrote: >> In insn: >> >> (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")))] >> >> one operand is (const_int 1). Its mode is VOIDmode. recog_data mode is >> VOIDmode. The patch takes right mode from PLUS containing it. > > Shouldn't this use a mode_iterator to put the right mode on the operand? That does sound better, and it would be nice to turn the genrecog warning: /* A modeless MATCH_OPERAND can be handy when we can check for multiple modes in the c_test. In most other cases, it is a mistake. Only DEFINE_INSN is eligible, since SPLIT and PEEP2 can FAIL within the output pattern. Exclude special predicates, which check the mode themselves. Also exclude predicates that allow only constants. Exclude the SET_DEST of a call instruction, as that is a common idiom. */ if (GET_MODE (pattern) == VOIDmode && code == MATCH_OPERAND && GET_CODE (insn) == DEFINE_INSN && pred && !pred->special && pred->allows_non_const && strstr (c_test, "operands") == NULL && ! (set && GET_CODE (set) == SET && GET_CODE (SET_SRC (set)) == CALL)) message_with_line (pattern_lineno, "warning: operand %d missing mode?", XINT (pattern, 0)); into an error to flush all these things out. I'm not sure it would help LRA or reload much though. (You might not have been implying otherwise, sorry.) We would still want to believe the mode of the matched operand over the mode of the match_operator for address operands, and still need to fall back on Pmode for const_int addresses, so I think the code would basically be the same. Richard
Index: lra-constraints.c =================================================================== --- lra-constraints.c (revision 192455) +++ lra-constraints.c (working copy) @@ -876,6 +876,65 @@ bitmap_head lra_bound_pseudos; (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) @@ -910,8 +969,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 = lra_get_mode (curr_static_id->operand[out].mode, out_rtx); - inmode = lra_get_mode (curr_static_id->operand[ins[0]].mode, in_rtx); + outmode = get_op_mode (out); + inmode = get_op_mode (ins[0]); if (inmode != outmode) { if (GET_MODE_SIZE (inmode) > GET_MODE_SIZE (outmode)) @@ -1639,8 +1698,7 @@ process_alt_operands (int only_alternati } op = no_subreg_reg_operand[nop]; - mode = lra_get_mode (curr_static_id->operand[nop].mode, - *curr_id->operand_loc[nop]); + mode = get_op_mode (nop); win = did_match = winreg = offmemok = constmemok = false; badop = true; @@ -2113,8 +2171,7 @@ process_alt_operands (int only_alternati && ((targetm.preferred_reload_class (op, this_alternative) == NO_REGS) || no_input_reloads_p) - && lra_get_mode (curr_static_id->operand[nop].mode, - op) != VOIDmode) + && get_op_mode (nop) != VOIDmode) { const_to_mem = 1; if (! no_regs_p) @@ -3099,8 +3156,7 @@ curr_insn_transform (void) rtx op = *curr_id->operand_loc[i]; rtx subreg = NULL_RTX; rtx plus = NULL_RTX; - enum machine_mode mode - = lra_get_mode (curr_static_id->operand[i].mode, op); + enum machine_mode mode = get_op_mode (i); if (GET_CODE (op) == SUBREG) { @@ -3214,7 +3270,7 @@ curr_insn_transform (void) enum op_type type = curr_static_id->operand[i].type; loc = curr_id->operand_loc[i]; - mode = lra_get_mode (curr_static_id->operand[i].mode, *loc); + mode = get_op_mode (i); if (GET_CODE (*loc) == SUBREG) { reg = SUBREG_REG (*loc); Index: lra-int.h =================================================================== --- lra-int.h (revision 192455) +++ lra-int.h (working copy) @@ -238,16 +238,6 @@ struct lra_insn_recog_data typedef struct lra_insn_recog_data *lra_insn_recog_data_t; -/* Return mode for X which corresponds to machine description operand - with mode MD_MODE. */ -static inline enum machine_mode -lra_get_mode (enum machine_mode md_mode, rtx x) -{ - if (GET_MODE (x) != VOIDmode) - return GET_MODE (x); - return md_mode; -} - /* lra.c: */ extern FILE *lra_dump_file;
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.