Patchwork [v2,rtl-optimization] : Fix PR46829, ICE in spill_failure with -fschedule-insns [was: Fixing instability of -fschedule-insns for x86]

login
register
mail settings
Submitter Uros Bizjak
Date Aug. 18, 2012, 8:14 a.m.
Message ID <CAFULd4bmbuFsVQfxtUvj4kfSQo_QbAbm3RMv312DM6VM9Jd6KA@mail.gmail.com>
Download mbox | patch
Permalink /patch/178435/
State New
Headers show

Comments

Uros Bizjak - Aug. 18, 2012, 8:14 a.m.
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.
Uros Bizjak - Aug. 18, 2012, 8:23 a.m.
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.
H.J. Lu - Aug. 18, 2012, 1:54 p.m.
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?
Oleg Endo - Aug. 20, 2012, 11:34 p.m.
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
Uros Bizjak - Aug. 22, 2012, 7:19 p.m.
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.

Patch

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;