Patchwork [lra] patch to revert a code from previous patch.

login
register
mail settings
Submitter Vladimir Makarov
Date Oct. 15, 2012, 5:39 p.m.
Message ID <507C4A38.3000005@redhat.com>
Download mbox | patch
Permalink /patch/191614/
State New
Headers show

Comments

Vladimir Makarov - Oct. 15, 2012, 5:39 p.m.
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.
Richard Sandiford - Oct. 15, 2012, 7:10 p.m.
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 - Oct. 15, 2012, 8:25 p.m.
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
Vladimir Makarov - Oct. 16, 2012, 2:30 a.m.
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.
Vladimir Makarov - Oct. 16, 2012, 2:32 a.m.
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.
Richard Sandiford - Oct. 16, 2012, 6:44 a.m.
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
Vladimir Makarov - Oct. 16, 2012, 3:06 p.m.
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.
Bernd Schmidt - Oct. 17, 2012, 1:35 p.m.
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
Richard Sandiford - Oct. 17, 2012, 2:30 p.m.
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

Patch

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;