| Message ID | CAFULd4bmbuFsVQfxtUvj4kfSQo_QbAbm3RMv312DM6VM9Jd6KA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Aug 18, 2012 at 10:14 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > After discussion with Oleg, it looks that it is enough to prevent > wrong registers in the output of the (multi-output) insn pattern. As > far as inputs are concerned, combine already handles limited reload > classes in the right way. The problem with x86 is, that reload tried > to fix output operand of the multi-output ins, where scheduler already > moved load of ax register before this insn. > > Version 2 of the patch now handles only output operands. Also, > handling of empty constraints was fixed. ... but here we fail testcase from PR46843... so we HAVE to check input operands. Uros.
On Sat, Aug 18, 2012 at 1:23 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Sat, Aug 18, 2012 at 10:14 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > >> After discussion with Oleg, it looks that it is enough to prevent >> wrong registers in the output of the (multi-output) insn pattern. As >> far as inputs are concerned, combine already handles limited reload >> classes in the right way. The problem with x86 is, that reload tried >> to fix output operand of the multi-output ins, where scheduler already >> moved load of ax register before this insn. >> >> Version 2 of the patch now handles only output operands. Also, >> handling of empty constraints was fixed. > > ... but here we fail testcase from PR46843... so we HAVE to check > input operands. > If this is very critical, can you add a target hook to decide whether to check input/output operands?
On Sat, 2012-08-18 at 10:23 +0200, Uros Bizjak wrote: > On Sat, Aug 18, 2012 at 10:14 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > > > After discussion with Oleg, it looks that it is enough to prevent > > wrong registers in the output of the (multi-output) insn pattern. As > > far as inputs are concerned, combine already handles limited reload > > classes in the right way. The problem with x86 is, that reload tried > > to fix output operand of the multi-output ins, where scheduler already > > moved load of ax register before this insn. > > > > Version 2 of the patch now handles only output operands. Also, > > handling of empty constraints was fixed. > > ... but here we fail testcase from PR46843... so we HAVE to check > input operands. Hm, I'm curious ... how would that work for patterns such as (define_insn "*addc" [(set (match_operand:SI 0 "arith_reg_dest" "=r") (plus:SI (plus:SI (match_operand:SI 1 "arith_reg_operand" "%0") (match_operand:SI 2 "arith_reg_or_0_operand" "r")) (match_operand:SI 3 "t_reg_operand" ""))) (clobber (reg:SI T_REG))] "TARGET_SH1" "addc %2,%0" [(set_attr "type" "arith")]) ... where the predicate "arith_reg_or_0_operand" allows either "const_int 0" or an "arith_reg_operand", but the constraint "r" tells reload to load the constant into a register for this insn. Probably those kind of patterns that rely on this behavior would need to be rewritten as insn_and_split to do the constant loading 'manually'? Cheers, Oleg
On Tue, Aug 21, 2012 at 1:34 AM, Oleg Endo <oleg.endo@t-online.de> wrote: > On Sat, 2012-08-18 at 10:23 +0200, Uros Bizjak wrote: >> On Sat, Aug 18, 2012 at 10:14 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> >> > After discussion with Oleg, it looks that it is enough to prevent >> > wrong registers in the output of the (multi-output) insn pattern. As >> > far as inputs are concerned, combine already handles limited reload >> > classes in the right way. The problem with x86 is, that reload tried >> > to fix output operand of the multi-output ins, where scheduler already >> > moved load of ax register before this insn. >> > >> > Version 2 of the patch now handles only output operands. Also, >> > handling of empty constraints was fixed. >> >> ... but here we fail testcase from PR46843... so we HAVE to check >> input operands. > > Hm, I'm curious ... how would that work for patterns such as > > (define_insn "*addc" > [(set (match_operand:SI 0 "arith_reg_dest" "=r") > (plus:SI (plus:SI > (match_operand:SI 1 "arith_reg_operand" "%0") > (match_operand:SI 2 "arith_reg_or_0_operand" "r")) > (match_operand:SI 3 "t_reg_operand" ""))) > (clobber (reg:SI T_REG))] > "TARGET_SH1" > "addc %2,%0" > [(set_attr "type" "arith")]) > > ... where the predicate "arith_reg_or_0_operand" allows either > "const_int 0" or an "arith_reg_operand", but the constraint "r" tells > reload to load the constant into a register for this insn. > Probably those kind of patterns that rely on this behavior would need to > be rewritten as insn_and_split to do the constant loading 'manually'? I think that we have to introduce a target hook that would "approve" the combined insn. This way, every target can analyse the combined insn and handle it in the most appropriate way. Uros.
Index: combine.c =================================================================== --- combine.c (revision 190500) +++ combine.c (working copy) @@ -10507,6 +10507,7 @@ recog_for_combine (rtx *pnewpat, rtx insn, rtx *pn int i; rtx notes = 0; rtx old_notes, old_pat; + int old_icode; /* If PAT is a PARALLEL, check to see if it contains the CLOBBER we use to indicate that something didn't match. If we find such a @@ -10566,6 +10567,7 @@ recog_for_combine (rtx *pnewpat, rtx insn, rtx *pn print_rtl_single (dump_file, pat); } } + PATTERN (insn) = old_pat; REG_NOTES (insn) = old_notes; @@ -10607,6 +10609,93 @@ recog_for_combine (rtx *pnewpat, rtx insn, rtx *pn pat = newpat; } + old_pat = PATTERN (insn); + old_notes = REG_NOTES (insn); + old_icode = INSN_CODE (insn); + PATTERN (insn) = pat; + REG_NOTES (insn) = notes; + + /* Check operand constraints in case wrong hard registers were + propagated into output operands of insn pattern. These invalid + insns can eventually confuse reload to error out with a + spill failure. See also PR 46829. */ + if (insn_code_number >= 0 + && insn_code_number != NOOP_MOVE_INSN_CODE + && (INSN_CODE (insn) = recog (PATTERN (insn), insn, 0)) >= 0) + { + extract_insn (insn); + preprocess_constraints (); + + for (i = 0; i < recog_data.n_operands; i++) + { + rtx op; + enum machine_mode mode; + struct operand_alternative *op_alt; + int offset = 0; + bool win; + int j; + + if (recog_data.operand_type[i] == OP_IN) + continue; + + op = recog_data.operand[i]; + mode = GET_MODE (op); + + /* A unary operator may be accepted by the predicate, but it + is irrelevant for matching constraints. */ + if (UNARY_P (op)) + op = XEXP (op, 0); + + if (GET_CODE (op) == SUBREG) + { + if (REG_P (SUBREG_REG (op)) + && REGNO (SUBREG_REG (op)) < FIRST_PSEUDO_REGISTER) + offset = subreg_regno_offset (REGNO (SUBREG_REG (op)), + GET_MODE (SUBREG_REG (op)), + SUBREG_BYTE (op), + GET_MODE (op)); + op = SUBREG_REG (op); + } + + if (!(REG_P (op) && HARD_REGISTER_P (op))) + continue; + + op_alt = recog_op_alt[i]; + + /* Operand has no constraints, anything is OK. */ + win = !recog_data.n_alternatives; + + for (j = 0; j < recog_data.n_alternatives; j++) + { + if (op_alt[j].anything_ok + || (op_alt[j].matches != -1 + && reg_fits_class_p (op, recog_op_alt[op_alt[j].matches][j].cl, + offset, mode)) + || reg_fits_class_p (op, op_alt[j].cl, offset, mode)) + { + win = true; + break; + } + } + + if (!win) + { + if (dump_file && (dump_flags & TDF_DETAILS)) + { + fputs ("Operand failed to match constraints:\n", + dump_file); + print_rtl_single (dump_file, recog_data.operand[i]); + } + insn_code_number = -1; + break; + } + } + } + + PATTERN (insn) = old_pat; + REG_NOTES (insn) = old_notes; + INSN_CODE (insn) = old_icode; + *pnewpat = pat; *pnotes = notes;
Hello! After discussion with Oleg, it looks that it is enough to prevent wrong registers in the output of the (multi-output) insn pattern. As far as inputs are concerned, combine already handles limited reload classes in the right way. The problem with x86 is, that reload tried to fix output operand of the multi-output ins, where scheduler already moved load of ax register before this insn. Version 2 of the patch now handles only output operands. Also, handling of empty constraints was fixed. 2012-08-18 Uros Bizjak <ubizjak@gmail.com> PR rtl-optimization/46829 * combine.c (recog_for_combine): Check operand constraints to reject instructions where wrong hard registers were propagated into output operands. testsuite/ChangeLog: 2012-08-18 Uros Bizjak <ubizjak@gmail.com> PR rtl-optimization/46829 * gcc.target/i386/pr46829.c: New test. Patch was bootstrapped and regression tested on x86_64-unknown-linux-gnu {,-m32}. Uros.