Remove mode argument from gen_rtx_SET

Remove mode argument from gen_rtx_SET

[Richard Sandiford]

[-- Attachment #1: Type: text/plain, Size: 5070 bytes --]

One problem with the automatically-generated gen_rtx_FOO () macros
is that they always have a mode parameter, even for codes like SET
where the mode should always be VOIDmode.  This inevitably leads to
cases where a caller accidentally passes something other than VOIDmode.
E.g. when expanding an SImode move, the temptation is to make everything
SImode, even the SETs.  This in turn can cause two instructions to appear
different simply because their SETs have different modes, even though the
SET_DEST and SET_SRC are identical.

E.g. for gcc/testsuite/g++.dg/torture/pr34651.C on lm32-elf we have
the following before jump2:

  (jump_insn 42 191 43 5 (set (pc)
	  (if_then_else (eq:SI (reg:SI 13 r13 [orig:43 inHotKey$4+-3 ] [43])
		  (const_int 0 [0]))
	      (label_ref 53)
	      (pc))) gcc/testsuite/g++.dg/torture/pr34651.C:22 22 {*beq}
       (expr_list:REG_DEAD (reg:SI 13 r13 [orig:43 inHotKey$4+-3 ] [43])
	  (int_list:REG_BR_PROB 5000 (nil)))
   -> 53)
  (note 43 42 48 6 [bb 6] NOTE_INSN_BASIC_BLOCK)
  (insn 48 43 47 6 (set (reg:SI 2 r2)
	  (mem/u/c:SI (reg:SI 1 r1) [4  S4 A32])) gcc/testsuite/g++.dg/torture/pr34651.C:22 7 {movsi_insn}
       (expr_list:REG_DEAD (reg:SI 1 r1)
	  (nil)))
  [...]
  (code_label 53 169 54 7 6 "" [1 uses])
  (note 54 53 12 7 [bb 7] NOTE_INSN_BASIC_BLOCK)
  (insn 12 54 57 7 (set:SI (reg/f:SI 2 r2 [orig:46 D.2050 ] [46])
	  (mem/u/c:SI (reg:SI 1 r1) [4  S4 A32])) gcc/testsuite/g++.dg/torture/pr34651.C:22 7 {movsi_insn}
       (expr_list:REG_DEAD (reg:SI 1 r1)
	  (expr_list:REG_EQUAL (symbol_ref/f:SI ("*.LC3") [flags 0x2]  <var_decl # *.LC3>)
	      (nil))))

where insns 12 and 48 are identical except for the :SI on the SET.
This difference prevents us from merging the heads of the two blocks;
after removing it we replace the two loads with a single load before
the branch.

This patch removes the mode argument from gen_rtx_SET and updates
all callers.  I used a script to (try to) make sure that all callers
really had been caught.  I also built one target per CPU just in case.
There were some changes in gcc.dg, g++.dg and gcc.c-torture assembly
code for c6x-elf, lm32-elf and v850-elf, but all of them seemed to be
code improvements from removing duplicated instructions.  (Other ports
also passed spurious modes but apparently not in a way that affects
the tests I'd tried.)  Also tested on x86_64-linux-gnu.  OK to install?

BTW, I've split the patch up into two, the last bit being a mechanical
removal of modes.  (I did it by hand though to try to keep things
properly formatted.)

Thanks,
Richard


gcc/
	* rtl.h (always_void_p): New function.
	* gengenrtl.c (always_void_p(: Likewise.
	(genmacro): Don't add a mode parameter to gen_rtx_foo if rtxes
	with code foo are always VOIDmode.
	* genemit.c (gen_exp): Update gen_rtx_foo calls accordingly.
	* builtins.c, caller-save.c, calls.c, cfgexpand.c, combine.c,
	compare-elim.c, config/aarch64/aarch64.c,
	config/aarch64/aarch64.md, config/alpha/alpha.c,
	config/alpha/alpha.md, config/arc/arc.c, config/arc/arc.md,
	config/arm/arm-fixed.md, config/arm/arm.c, config/arm/arm.md,
	config/arm/ldrdstrd.md, config/arm/thumb2.md, config/arm/vfp.md,
	config/avr/avr.c, config/bfin/bfin.c, config/c6x/c6x.c,
	config/c6x/c6x.md, config/cr16/cr16.c, config/cris/cris.c,
	config/cris/cris.md, config/darwin.c, config/epiphany/epiphany.c,
	config/epiphany/epiphany.md, config/fr30/fr30.c, config/frv/frv.c,
	config/frv/frv.md, config/h8300/h8300.c, config/i386/i386.c,
	config/i386/i386.md, config/i386/sse.md, config/ia64/ia64.c,
	config/ia64/vect.md, config/iq2000/iq2000.c,
	config/iq2000/iq2000.md, config/lm32/lm32.c, config/lm32/lm32.md,
	config/m32c/m32c.c, config/m32r/m32r.c, config/m68k/m68k.c,
	config/m68k/m68k.md, config/mcore/mcore.c, config/mcore/mcore.md,
	config/mep/mep.c, config/microblaze/microblaze.c,
	config/mips/mips.c, config/mips/mips.md, config/mmix/mmix.c,
	config/mn10300/mn10300.c, config/msp430/msp430.c,
	config/nds32/nds32-memory-manipulation.c, config/nds32/nds32.c,
	config/nds32/nds32.md, config/nios2/nios2.c, config/nvptx/nvptx.c,
	config/pa/pa.c, config/pa/pa.md, config/rl78/rl78.c,
	config/rs6000/altivec.md, config/rs6000/rs6000.c,
	config/rs6000/rs6000.md, config/rs6000/vector.md,
	config/rs6000/vsx.md, config/rx/rx.c, config/rx/rx.md,
	config/s390/s390.c, config/s390/s390.md, config/sh/sh.c,
	config/sh/sh.md, config/sh/sh_treg_combine.cc,
	config/sparc/sparc.c, config/sparc/sparc.md, config/spu/spu.c,
	config/spu/spu.md, config/stormy16/stormy16.c,
	config/tilegx/tilegx.c, config/tilegx/tilegx.md,
	config/tilepro/tilepro.c, config/tilepro/tilepro.md,
	config/v850/v850.c, config/v850/v850.md, config/vax/vax.c,
	config/visium/visium.c, config/xtensa/xtensa.c, cprop.c, dse.c,
	expr.c, gcse.c, ifcvt.c, ira.c, jump.c, lower-subreg.c,
	lra-constraints.c, lra-eliminations.c, lra.c, postreload.c, ree.c,
	reg-stack.c, reload.c, reload1.c, reorg.c, sel-sched.c,
	var-tracking.c: Update gen_rtx_SET calls accordingly.


[-- Attachment #2: mechanical-part.diff.bz2 --]
[-- Type: application/octet-stream, Size: 63017 bytes --]

Re: Remove mode argument from gen_rtx_SET

[Jeff Law]

On 05/07/2015 05:37 AM, Richard Sandiford wrote:
> One problem with the automatically-generated gen_rtx_FOO () macros
> is that they always have a mode parameter, even for codes like SET
> where the mode should always be VOIDmode.  This inevitably leads to
> cases where a caller accidentally passes something other than VOIDmode.
> E.g. when expanding an SImode move, the temptation is to make everything
> SImode, even the SETs.  This in turn can cause two instructions to appear
> different simply because their SETs have different modes, even though the
> SET_DEST and SET_SRC are identical.
>
> E.g. for gcc/testsuite/g++.dg/torture/pr34651.C on lm32-elf we have
> the following before jump2:
>
>    (jump_insn 42 191 43 5 (set (pc)
> 	  (if_then_else (eq:SI (reg:SI 13 r13 [orig:43 inHotKey$4+-3 ] [43])
> 		  (const_int 0 [0]))
> 	      (label_ref 53)
> 	      (pc))) gcc/testsuite/g++.dg/torture/pr34651.C:22 22 {*beq}
>         (expr_list:REG_DEAD (reg:SI 13 r13 [orig:43 inHotKey$4+-3 ] [43])
> 	  (int_list:REG_BR_PROB 5000 (nil)))
>     -> 53)
>    (note 43 42 48 6 [bb 6] NOTE_INSN_BASIC_BLOCK)
>    (insn 48 43 47 6 (set (reg:SI 2 r2)
> 	  (mem/u/c:SI (reg:SI 1 r1) [4  S4 A32])) gcc/testsuite/g++.dg/torture/pr34651.C:22 7 {movsi_insn}
>         (expr_list:REG_DEAD (reg:SI 1 r1)
> 	  (nil)))
>    [...]
>    (code_label 53 169 54 7 6 "" [1 uses])
>    (note 54 53 12 7 [bb 7] NOTE_INSN_BASIC_BLOCK)
>    (insn 12 54 57 7 (set:SI (reg/f:SI 2 r2 [orig:46 D.2050 ] [46])
> 	  (mem/u/c:SI (reg:SI 1 r1) [4  S4 A32])) gcc/testsuite/g++.dg/torture/pr34651.C:22 7 {movsi_insn}
>         (expr_list:REG_DEAD (reg:SI 1 r1)
> 	  (expr_list:REG_EQUAL (symbol_ref/f:SI ("*.LC3") [flags 0x2]  <var_decl # *.LC3>)
> 	      (nil))))
>
> where insns 12 and 48 are identical except for the :SI on the SET.
> This difference prevents us from merging the heads of the two blocks;
> after removing it we replace the two loads with a single load before
> the branch.
>
> This patch removes the mode argument from gen_rtx_SET and updates
> all callers.  I used a script to (try to) make sure that all callers
> really had been caught.  I also built one target per CPU just in case.
> There were some changes in gcc.dg, g++.dg and gcc.c-torture assembly
> code for c6x-elf, lm32-elf and v850-elf, but all of them seemed to be
> code improvements from removing duplicated instructions.  (Other ports
> also passed spurious modes but apparently not in a way that affects
> the tests I'd tried.)  Also tested on x86_64-linux-gnu.  OK to install?
>
> BTW, I've split the patch up into two, the last bit being a mechanical
> removal of modes.  (I did it by hand though to try to keep things
> properly formatted.)
>
> Thanks,
> Richard
>
>
> gcc/
> 	* rtl.h (always_void_p): New function.
> 	* gengenrtl.c (always_void_p(: Likewise.
Typo in the ChangeLog.

non-mechanical part didn't seem to be attached.  I don't expect any 
problems there, but a quick looksie seems appropriate.

Mechanical bits are obviously OK once the non-mechanical bits are in place.

jeff

Re: Remove mode argument from gen_rtx_SET

[Richard Sandiford]

Jeff Law <law@redhat.com> writes:
> On 05/07/2015 05:37 AM, Richard Sandiford wrote:
>> One problem with the automatically-generated gen_rtx_FOO () macros
>> is that they always have a mode parameter, even for codes like SET
>> where the mode should always be VOIDmode.  This inevitably leads to
>> cases where a caller accidentally passes something other than VOIDmode.
>> E.g. when expanding an SImode move, the temptation is to make everything
>> SImode, even the SETs.  This in turn can cause two instructions to appear
>> different simply because their SETs have different modes, even though the
>> SET_DEST and SET_SRC are identical.
>>
>> E.g. for gcc/testsuite/g++.dg/torture/pr34651.C on lm32-elf we have
>> the following before jump2:
>>
>>    (jump_insn 42 191 43 5 (set (pc)
>> 	  (if_then_else (eq:SI (reg:SI 13 r13 [orig:43 inHotKey$4+-3 ] [43])
>> 		  (const_int 0 [0]))
>> 	      (label_ref 53)
>> 	      (pc))) gcc/testsuite/g++.dg/torture/pr34651.C:22 22 {*beq}
>>         (expr_list:REG_DEAD (reg:SI 13 r13 [orig:43 inHotKey$4+-3 ] [43])
>> 	  (int_list:REG_BR_PROB 5000 (nil)))
>>     -> 53)
>>    (note 43 42 48 6 [bb 6] NOTE_INSN_BASIC_BLOCK)
>>    (insn 48 43 47 6 (set (reg:SI 2 r2)
>> 	  (mem/u/c:SI (reg:SI 1 r1) [4  S4 A32])) gcc/testsuite/g++.dg/torture/pr34651.C:22 7 {movsi_insn}
>>         (expr_list:REG_DEAD (reg:SI 1 r1)
>> 	  (nil)))
>>    [...]
>>    (code_label 53 169 54 7 6 "" [1 uses])
>>    (note 54 53 12 7 [bb 7] NOTE_INSN_BASIC_BLOCK)
>>    (insn 12 54 57 7 (set:SI (reg/f:SI 2 r2 [orig:46 D.2050 ] [46])
>> 	  (mem/u/c:SI (reg:SI 1 r1) [4  S4 A32])) gcc/testsuite/g++.dg/torture/pr34651.C:22 7 {movsi_insn}
>>         (expr_list:REG_DEAD (reg:SI 1 r1)
>> 	  (expr_list:REG_EQUAL (symbol_ref/f:SI ("*.LC3") [flags 0x2]  <var_decl # *.LC3>)
>> 	      (nil))))
>>
>> where insns 12 and 48 are identical except for the :SI on the SET.
>> This difference prevents us from merging the heads of the two blocks;
>> after removing it we replace the two loads with a single load before
>> the branch.
>>
>> This patch removes the mode argument from gen_rtx_SET and updates
>> all callers.  I used a script to (try to) make sure that all callers
>> really had been caught.  I also built one target per CPU just in case.
>> There were some changes in gcc.dg, g++.dg and gcc.c-torture assembly
>> code for c6x-elf, lm32-elf and v850-elf, but all of them seemed to be
>> code improvements from removing duplicated instructions.  (Other ports
>> also passed spurious modes but apparently not in a way that affects
>> the tests I'd tried.)  Also tested on x86_64-linux-gnu.  OK to install?
>>
>> BTW, I've split the patch up into two, the last bit being a mechanical
>> removal of modes.  (I did it by hand though to try to keep things
>> properly formatted.)
>>
>> Thanks,
>> Richard
>>
>>
>> gcc/
>> 	* rtl.h (always_void_p): New function.
>> 	* gengenrtl.c (always_void_p(: Likewise.
> Typo in the ChangeLog.
>
> non-mechanical part didn't seem to be attached.  I don't expect any 
> problems there, but a quick looksie seems appropriate.

Bah.  Sorry about that.

It's unfortunate that we have two copies of the test -- one on codes
and one on strings -- but that's how everything else in gengenrtl.c
is done.  Maybe a later clean-up.


Index: gcc/genemit.c
===================================================================
--- gcc/genemit.c	2015-05-07 07:59:47.890577327 +0100
+++ gcc/genemit.c	2015-05-07 07:59:47.874577523 +0100
@@ -94,6 +94,7 @@ gen_exp (rtx x, enum rtx_code subroutine
   int i;
   int len;
   const char *fmt;
+  const char *sep = "";
 
   if (x == 0)
     {
@@ -215,7 +216,12 @@ gen_exp (rtx x, enum rtx_code subroutine
 
   printf ("gen_rtx_");
   print_code (code);
-  printf (" (%smode", GET_MODE_NAME (GET_MODE (x)));
+  printf (" (");
+  if (!always_void_p (code))
+    {
+      printf ("%smode", GET_MODE_NAME (GET_MODE (x)));
+      sep = ",\n\t";
+    }
 
   fmt = GET_RTX_FORMAT (code);
   len = GET_RTX_LENGTH (code);
@@ -223,7 +229,7 @@ gen_exp (rtx x, enum rtx_code subroutine
     {
       if (fmt[i] == '0')
 	break;
-      printf (",\n\t");
+      fputs (sep, stdout);
       switch (fmt[i])
 	{
 	case 'e': case 'u':
@@ -254,6 +260,7 @@ gen_exp (rtx x, enum rtx_code subroutine
 	default:
 	  gcc_unreachable ();
 	}
+      sep = ",\n\t";
     }
   printf (")");
 }
Index: gcc/gengenrtl.c
===================================================================
--- gcc/gengenrtl.c	2015-05-07 07:59:47.890577327 +0100
+++ gcc/gengenrtl.c	2015-05-07 08:12:24.281310491 +0100
@@ -116,6 +116,14 @@ special_format (const char *fmt)
 	  || strchr (fmt, 'n') != 0);
 }
 
+/* Return true if CODE always has VOIDmode.  */
+
+static inline bool
+always_void_p (int idx)
+{
+  return strcmp (defs[idx].enumname, "SET") == 0;
+}
+
 /* Return nonzero if the RTL code given by index IDX is one that we should
    generate a gen_rtx_raw_FOO macro for, not gen_rtx_FOO (because gen_rtx_FOO
    is a wrapper in emit-rtl.c).  */
@@ -181,6 +189,7 @@ find_formats (void)
 genmacro (int idx)
 {
   const char *p;
+  const char *sep = "";
   int i;
 
   /* We write a macro that defines gen_rtx_RTLCODE to be an equivalent to
@@ -190,15 +199,25 @@ genmacro (int idx)
     /* Don't define a macro for this code.  */
     return;
 
-  printf ("#define gen_rtx_%s%s(MODE",
+  bool has_mode_p = !always_void_p (idx);
+  printf ("#define gen_rtx_%s%s(",
 	   special_rtx (idx) ? "raw_" : "", defs[idx].enumname);
+  if (has_mode_p)
+    {
+      printf ("MODE");
+      sep = ", ";
+    }
 
   for (p = defs[idx].format, i = 0; *p != 0; p++)
     if (*p != '0')
-      printf (", ARG%d", i++);
-
-  printf (") \\\n  gen_rtx_fmt_%s (%s, (MODE)",
-	  defs[idx].format, defs[idx].enumname);
+      {
+	printf ("%sARG%d", sep, i++);
+	sep = ", ";
+      }
+
+  printf (") \\\n  gen_rtx_fmt_%s (%s, %s",
+	  defs[idx].format, defs[idx].enumname,
+	  has_mode_p ? "(MODE)" : "VOIDmode");
 
   for (p = defs[idx].format, i = 0; *p != 0; p++)
     if (*p != '0')
Index: gcc/rtl.h
===================================================================
--- gcc/rtl.h	2015-05-07 07:59:47.890577327 +0100
+++ gcc/rtl.h	2015-05-07 08:11:56.405652046 +0100
@@ -1787,6 +1787,14 @@ #define COSTS_N_INSNS(N) ((N) * 4)
    not to use an rtx with this cost under any circumstances.  */
 #define MAX_COST INT_MAX
 
+/* Return true if CODE always has VOIDmode.  */
+
+static inline bool
+always_void_p (enum rtx_code code)
+{
+  return code == SET;
+}
+
 /* A structure to hold all available cost information about an rtl
    expression.  */
 struct full_rtx_costs
@@ -3597,5 +3605,4 @@ #define fatal_insn_not_found(insn) \
 /* reginfo.c */
 extern tree GTY(()) global_regs_decl[FIRST_PSEUDO_REGISTER];
 
-
 #endif /* ! GCC_RTL_H */

Re: Remove mode argument from gen_rtx_SET

[Jeff Law]

On 05/07/2015 07:59 AM, Richard Sandiford wrote:
> Jeff Law <law@redhat.com> writes:
>> On 05/07/2015 05:37 AM, Richard Sandiford wrote:
>>> One problem with the automatically-generated gen_rtx_FOO () macros
>>> is that they always have a mode parameter, even for codes like SET
>>> where the mode should always be VOIDmode.  This inevitably leads to
>>> cases where a caller accidentally passes something other than VOIDmode.
>>> E.g. when expanding an SImode move, the temptation is to make everything
>>> SImode, even the SETs.  This in turn can cause two instructions to appear
>>> different simply because their SETs have different modes, even though the
>>> SET_DEST and SET_SRC are identical.
>>>
>>> E.g. for gcc/testsuite/g++.dg/torture/pr34651.C on lm32-elf we have
>>> the following before jump2:
>>>
>>>     (jump_insn 42 191 43 5 (set (pc)
>>> 	  (if_then_else (eq:SI (reg:SI 13 r13 [orig:43 inHotKey$4+-3 ] [43])
>>> 		  (const_int 0 [0]))
>>> 	      (label_ref 53)
>>> 	      (pc))) gcc/testsuite/g++.dg/torture/pr34651.C:22 22 {*beq}
>>>          (expr_list:REG_DEAD (reg:SI 13 r13 [orig:43 inHotKey$4+-3 ] [43])
>>> 	  (int_list:REG_BR_PROB 5000 (nil)))
>>>      -> 53)
>>>     (note 43 42 48 6 [bb 6] NOTE_INSN_BASIC_BLOCK)
>>>     (insn 48 43 47 6 (set (reg:SI 2 r2)
>>> 	  (mem/u/c:SI (reg:SI 1 r1) [4  S4 A32])) gcc/testsuite/g++.dg/torture/pr34651.C:22 7 {movsi_insn}
>>>          (expr_list:REG_DEAD (reg:SI 1 r1)
>>> 	  (nil)))
>>>     [...]
>>>     (code_label 53 169 54 7 6 "" [1 uses])
>>>     (note 54 53 12 7 [bb 7] NOTE_INSN_BASIC_BLOCK)
>>>     (insn 12 54 57 7 (set:SI (reg/f:SI 2 r2 [orig:46 D.2050 ] [46])
>>> 	  (mem/u/c:SI (reg:SI 1 r1) [4  S4 A32])) gcc/testsuite/g++.dg/torture/pr34651.C:22 7 {movsi_insn}
>>>          (expr_list:REG_DEAD (reg:SI 1 r1)
>>> 	  (expr_list:REG_EQUAL (symbol_ref/f:SI ("*.LC3") [flags 0x2]  <var_decl # *.LC3>)
>>> 	      (nil))))
>>>
>>> where insns 12 and 48 are identical except for the :SI on the SET.
>>> This difference prevents us from merging the heads of the two blocks;
>>> after removing it we replace the two loads with a single load before
>>> the branch.
>>>
>>> This patch removes the mode argument from gen_rtx_SET and updates
>>> all callers.  I used a script to (try to) make sure that all callers
>>> really had been caught.  I also built one target per CPU just in case.
>>> There were some changes in gcc.dg, g++.dg and gcc.c-torture assembly
>>> code for c6x-elf, lm32-elf and v850-elf, but all of them seemed to be
>>> code improvements from removing duplicated instructions.  (Other ports
>>> also passed spurious modes but apparently not in a way that affects
>>> the tests I'd tried.)  Also tested on x86_64-linux-gnu.  OK to install?
>>>
>>> BTW, I've split the patch up into two, the last bit being a mechanical
>>> removal of modes.  (I did it by hand though to try to keep things
>>> properly formatted.)
>>>
>>> Thanks,
>>> Richard
>>>
>>>
>>> gcc/
>>> 	* rtl.h (always_void_p): New function.
>>> 	* gengenrtl.c (always_void_p(: Likewise.
>> Typo in the ChangeLog.
>>
>> non-mechanical part didn't seem to be attached.  I don't expect any
>> problems there, but a quick looksie seems appropriate.
>
> Bah.  Sorry about that.
>
> It's unfortunate that we have two copies of the test -- one on codes
> and one on strings -- but that's how everything else in gengenrtl.c
> is done.  Maybe a later clean-up.
Looks good to me.

I wonder if there's others with similar characteristics...  The toplevel 
objects come to mind, but ultimately we don't want them to be RTXs at 
all IMHO.

Anyway, it's a good cleanup unto itself and clearly folks have gotten it 
wrong in the past based on your testing results.  Thanks for taking care 
of it.

Jeff

Re: Remove mode argument from gen_rtx_SET

[Franz Sirl]

Am 2015-05-07 um 13:37 schrieb Richard Sandiford:
> One problem with the automatically-generated gen_rtx_FOO () macros
> is that they always have a mode parameter, even for codes like SET
> where the mode should always be VOIDmode.  This inevitably leads to
> cases where a caller accidentally passes something other than VOIDmode.
> E.g. when expanding an SImode move, the temptation is to make everything
> SImode, even the SETs.  This in turn can cause two instructions to appear
> different simply because their SETs have different modes, even though the
> SET_DEST and SET_SRC are identical.
>
> E.g. for gcc/testsuite/g++.dg/torture/pr34651.C on lm32-elf we have
> the following before jump2:
>
>    (jump_insn 42 191 43 5 (set (pc)
> 	  (if_then_else (eq:SI (reg:SI 13 r13 [orig:43 inHotKey$4+-3 ] [43])
> 		  (const_int 0 [0]))
> 	      (label_ref 53)
> 	      (pc))) gcc/testsuite/g++.dg/torture/pr34651.C:22 22 {*beq}
>         (expr_list:REG_DEAD (reg:SI 13 r13 [orig:43 inHotKey$4+-3 ] [43])
> 	  (int_list:REG_BR_PROB 5000 (nil)))
>     -> 53)
>    (note 43 42 48 6 [bb 6] NOTE_INSN_BASIC_BLOCK)
>    (insn 48 43 47 6 (set (reg:SI 2 r2)
> 	  (mem/u/c:SI (reg:SI 1 r1) [4  S4 A32])) gcc/testsuite/g++.dg/torture/pr34651.C:22 7 {movsi_insn}
>         (expr_list:REG_DEAD (reg:SI 1 r1)
> 	  (nil)))
>    [...]
>    (code_label 53 169 54 7 6 "" [1 uses])
>    (note 54 53 12 7 [bb 7] NOTE_INSN_BASIC_BLOCK)
>    (insn 12 54 57 7 (set:SI (reg/f:SI 2 r2 [orig:46 D.2050 ] [46])
> 	  (mem/u/c:SI (reg:SI 1 r1) [4  S4 A32])) gcc/testsuite/g++.dg/torture/pr34651.C:22 7 {movsi_insn}
>         (expr_list:REG_DEAD (reg:SI 1 r1)
> 	  (expr_list:REG_EQUAL (symbol_ref/f:SI ("*.LC3") [flags 0x2]  <var_decl # *.LC3>)
> 	      (nil))))
>
> where insns 12 and 48 are identical except for the :SI on the SET.
> This difference prevents us from merging the heads of the two blocks;
> after removing it we replace the two loads with a single load before
> the branch.
>
> This patch removes the mode argument from gen_rtx_SET and updates
> all callers.  I used a script to (try to) make sure that all callers
> really had been caught.  I also built one target per CPU just in case.
> There were some changes in gcc.dg, g++.dg and gcc.c-torture assembly
> code for c6x-elf, lm32-elf and v850-elf, but all of them seemed to be
> code improvements from removing duplicated instructions.  (Other ports
> also passed spurious modes but apparently not in a way that affects
> the tests I'd tried.)  Also tested on x86_64-linux-gnu.  OK to install?
>
> BTW, I've split the patch up into two, the last bit being a mechanical
> removal of modes.  (I did it by hand though to try to keep things
> properly formatted.)
>
> Thanks,
> Richard
>
>
> gcc/
> 	* rtl.h (always_void_p): New function.
> 	* gengenrtl.c (always_void_p(: Likewise.
> 	(genmacro): Don't add a mode parameter to gen_rtx_foo if rtxes
> 	with code foo are always VOIDmode.
> 	* genemit.c (gen_exp): Update gen_rtx_foo calls accordingly.
> 	* builtins.c, caller-save.c, calls.c, cfgexpand.c, combine.c,
> 	compare-elim.c, config/aarch64/aarch64.c,
> 	config/aarch64/aarch64.md, config/alpha/alpha.c,
> 	config/alpha/alpha.md, config/arc/arc.c, config/arc/arc.md,
> 	config/arm/arm-fixed.md, config/arm/arm.c, config/arm/arm.md,
> 	config/arm/ldrdstrd.md, config/arm/thumb2.md, config/arm/vfp.md,
> 	config/avr/avr.c, config/bfin/bfin.c, config/c6x/c6x.c,
> 	config/c6x/c6x.md, config/cr16/cr16.c, config/cris/cris.c,
> 	config/cris/cris.md, config/darwin.c, config/epiphany/epiphany.c,
> 	config/epiphany/epiphany.md, config/fr30/fr30.c, config/frv/frv.c,
> 	config/frv/frv.md, config/h8300/h8300.c, config/i386/i386.c,
> 	config/i386/i386.md, config/i386/sse.md, config/ia64/ia64.c,
> 	config/ia64/vect.md, config/iq2000/iq2000.c,
> 	config/iq2000/iq2000.md, config/lm32/lm32.c, config/lm32/lm32.md,
> 	config/m32c/m32c.c, config/m32r/m32r.c, config/m68k/m68k.c,
> 	config/m68k/m68k.md, config/mcore/mcore.c, config/mcore/mcore.md,
> 	config/mep/mep.c, config/microblaze/microblaze.c,
> 	config/mips/mips.c, config/mips/mips.md, config/mmix/mmix.c,
> 	config/mn10300/mn10300.c, config/msp430/msp430.c,
> 	config/nds32/nds32-memory-manipulation.c, config/nds32/nds32.c,
> 	config/nds32/nds32.md, config/nios2/nios2.c, config/nvptx/nvptx.c,
> 	config/pa/pa.c, config/pa/pa.md, config/rl78/rl78.c,
> 	config/rs6000/altivec.md, config/rs6000/rs6000.c,
> 	config/rs6000/rs6000.md, config/rs6000/vector.md,
> 	config/rs6000/vsx.md, config/rx/rx.c, config/rx/rx.md,
> 	config/s390/s390.c, config/s390/s390.md, config/sh/sh.c,
> 	config/sh/sh.md, config/sh/sh_treg_combine.cc,
> 	config/sparc/sparc.c, config/sparc/sparc.md, config/spu/spu.c,
> 	config/spu/spu.md, config/stormy16/stormy16.c,
> 	config/tilegx/tilegx.c, config/tilegx/tilegx.md,
> 	config/tilepro/tilepro.c, config/tilepro/tilepro.md,
> 	config/v850/v850.c, config/v850/v850.md, config/vax/vax.c,
> 	config/visium/visium.c, config/xtensa/xtensa.c, cprop.c, dse.c,
> 	expr.c, gcse.c, ifcvt.c, ira.c, jump.c, lower-subreg.c,
> 	lra-constraints.c, lra-eliminations.c, lra.c, postreload.c, ree.c,
> 	reg-stack.c, reload.c, reload1.c, reorg.c, sel-sched.c,
> 	var-tracking.c: Update gen_rtx_SET calls accordingly.
>

Hi Richard,

this patch (r222882 is fine, r222883 fails) breaks bootstrap for me on 
x86_64-linux-gnu:

libtool: compile: 
/home/fsirl/BUILD/gcc-6.0.0-r222903/obj-x86_64-suse-linux/./gcc/xgcc 
-B/home/fsirl/BUILD/gcc-6.0.0-r222903/obj-x86_64-suse-linux/./gcc/ 
-B/usr/x86_64-suse-linux/bin/ -B/
usr/x86_64-suse-linux/lib/ -isystem /usr/x86_64-suse-linux/include 
-isystem /usr/x86_64-suse-linux/sys-include -DHAVE_CONFIG_H -I. 
-I../../../../libmpx/mpxwrap -I.. -fcheck-pointer-bounds
-mmpx -fno-chkp-check-read -fno-chkp-check-write -fno-chkp-use-wrappers 
-fPIC -O2 -g -fmessage-length=0 -D_FORTIFY_SOURCE=2 -funwind-tables 
-fasynchronous-unwind-tables -U_FORTIFY_SOURCE -
c ../../../../libmpx/mpxwrap/mpx_wrappers.c  -fPIC -DPIC -o 
.libs/libmpxwrappers_la-mpx_wrappers.o
../../../../libmpx/mpxwrap/mpx_wrappers.c: In function 
‘__mpx_wrapper_memmove.chkp’:
../../../../libmpx/mpxwrap/mpx_wrappers.c:160:1: error: unrecognizable insn:
  }
  ^
(insn 175 174 176 15 (parallel [
             (set (reg:BND64 175 [ __bound_tmp.21 ])
                 (unspec:BND64 [
                         (mem:DI (unspec:DI [
                                     (reg/v/f:DI 158 [ s ])
                                     (reg/f:DI 159 [ D.3339 ])
                                 ] UNSPEC_BNDLDX_ADDR) [0  S8 A8])
                     ] UNSPEC_BNDLDX))
             (use (mem:BLK (reg/v/f:DI 158 [ s ]) [0  A8]))
         ]) -1
      (nil))
../../../../libmpx/mpxwrap/mpx_wrappers.c:160:1: internal compiler 
error: in extract_insn, at recog.c:2341
0x982ee5 _fatal_insn(char const*, rtx_def const*, char const*, int, char 
const*)
         ../../gcc/rtl-error.c:110
0x982f23 _fatal_insn_not_found(rtx_def const*, char const*, int, char 
const*)
         ../../gcc/rtl-error.c:118
0x933b92 extract_insn(rtx_insn*)
         ../../gcc/recog.c:2341
0x6c09bf instantiate_virtual_regs_in_insn
         ../../gcc/function.c:1598
0x6c1273 instantiate_virtual_regs
         ../../gcc/function.c:1966
0x6c1304 execute
         ../../gcc/function.c:2015

Franz.

Re: Remove mode argument from gen_rtx_SET

[Segher Boessenkool]

On Fri, May 08, 2015 at 12:32:30PM +0200, Franz Sirl wrote:
> this patch (r222882 is fine, r222883 fails) breaks bootstrap for me on 
> x86_64-linux-gnu:

i386.md has "set:BND" twice; replace that with just "set", and all
should be fine.

Maybe gen* should warn on this; maybe it already does.


Segher

Re: Remove mode argument from gen_rtx_SET

[Franz Sirl]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

Am 2015-05-08 um 13:57 schrieb Segher Boessenkool:
> On Fri, May 08, 2015 at 12:32:30PM +0200, Franz Sirl wrote:
>> this patch (r222882 is fine, r222883 fails) breaks bootstrap for me on
>> x86_64-linux-gnu:
>
> i386.md has "set:BND" twice; replace that with just "set", and all
> should be fine.
>
> Maybe gen* should warn on this; maybe it already does.

I didn't see a warning in the logs at least. But your suggestion fixes 
the bootstrap for me.

Franz.






[-- Attachment #2: gcc-r222883-fix.patch --]
[-- Type: text/plain, Size: 806 bytes --]

Index: gcc/config/i386/i386.md
===================================================================
--- gcc/config/i386/i386.md	(revision 222909)
+++ gcc/config/i386/i386.md	(working copy)
@@ -18879,7 +18879,7 @@
   [(set_attr "type" "mpxchk")])
 
 (define_expand "<mode>_ldx"
-  [(parallel [(set:BND (match_operand:BND 0 "register_operand")
+  [(parallel [(set (match_operand:BND 0 "register_operand")
                        (unspec:BND
 		         [(mem:<bnd_ptr>
 			   (match_par_dup 3
@@ -18909,7 +18909,7 @@
 })
 
 (define_insn "*<mode>_ldx"
-  [(parallel [(set:BND (match_operand:BND 0 "register_operand" "=w")
+  [(parallel [(set (match_operand:BND 0 "register_operand" "=w")
                        (unspec:BND
 		         [(match_operator:<bnd_ptr> 3 "bnd_mem_operator"
 			   [(unspec:<bnd_ptr>

Re: Remove mode argument from gen_rtx_SET

[Richard Sandiford]

Franz Sirl <Franz.Sirl-kernel@lauterbach.com> writes:
> Am 2015-05-08 um 13:57 schrieb Segher Boessenkool:
>> On Fri, May 08, 2015 at 12:32:30PM +0200, Franz Sirl wrote:
>>> this patch (r222882 is fine, r222883 fails) breaks bootstrap for me on
>>> x86_64-linux-gnu:
>>
>> i386.md has "set:BND" twice; replace that with just "set", and all
>> should be fine.
>>
>> Maybe gen* should warn on this; maybe it already does.
>
> I didn't see a warning in the logs at least. But your suggestion fixes 
> the bootstrap for me.

Thanks.  I installed this as obvious after testing that x86_64-linux-gnu
built with --enable-libmpx and that rx-elf could handle:

  void f(long long *a) { a[0] = a[1]; }

when -mlra was passed.

There's also one in a comment in msp430.md:

; This pattern is identical to the truncsipsi2 pattern except
; that it uses a SUBREG instead of a TRUNC.  It is needed in
; order to prevent reload from converting (set:SI (SUBREG:PSI (SI)))
; into (SET:PSI (PSI)).

I'm not sure what that's supposed to mean (what's an SI set of a PSI
subreg?), but I suspect removing the mode would lose information,
so I left it alone.

I'll follow up with a patch to make the generators raise an error
for this, as well as to restore the "missing mode" diagnostics
mentioned in the genrecog thread.

Sorry for the breakage.

Richard


gcc/
	* config/i386/i386.md (<mode>_ldx, *<mode>_ldx): Remove mode
	from (set ...).
	* config/rx/rx.md (movdi, movdf): Likewise.
	Likewise for define_peephole2s.

Index: gcc/config/i386/i386.md
===================================================================
--- gcc/config/i386/i386.md	2015-05-08 14:42:57.823310127 +0100
+++ gcc/config/i386/i386.md	2015-05-08 14:43:12.515140307 +0100
@@ -18879,13 +18879,13 @@ (define_insn "*<mode>_<bndcheck>"
   [(set_attr "type" "mpxchk")])
 
 (define_expand "<mode>_ldx"
-  [(parallel [(set:BND (match_operand:BND 0 "register_operand")
-                       (unspec:BND
-		         [(mem:<bnd_ptr>
-			   (match_par_dup 3
-			     [(match_operand:<bnd_ptr> 1 "address_mpx_no_index_operand")
-	                      (match_operand:<bnd_ptr> 2 "register_operand")]))]
-			 UNSPEC_BNDLDX))
+  [(parallel [(set (match_operand:BND 0 "register_operand")
+                   (unspec:BND
+		     [(mem:<bnd_ptr>
+		       (match_par_dup 3
+			 [(match_operand:<bnd_ptr> 1 "address_mpx_no_index_operand")
+			  (match_operand:<bnd_ptr> 2 "register_operand")]))]
+		     UNSPEC_BNDLDX))
               (use (mem:BLK (match_dup 1)))])]
   "TARGET_MPX"
 {
@@ -18909,14 +18909,14 @@ (define_expand "<mode>_ldx"
 })
 
 (define_insn "*<mode>_ldx"
-  [(parallel [(set:BND (match_operand:BND 0 "register_operand" "=w")
-                       (unspec:BND
-		         [(match_operator:<bnd_ptr> 3 "bnd_mem_operator"
-			   [(unspec:<bnd_ptr>
-			     [(match_operand:<bnd_ptr> 1 "address_mpx_no_index_operand" "Ti")
-	                      (match_operand:<bnd_ptr> 2 "register_operand" "l")]
-			    UNSPEC_BNDLDX_ADDR)])]
-			 UNSPEC_BNDLDX))
+  [(parallel [(set (match_operand:BND 0 "register_operand" "=w")
+		   (unspec:BND
+		     [(match_operator:<bnd_ptr> 3 "bnd_mem_operator"
+		       [(unspec:<bnd_ptr>
+			 [(match_operand:<bnd_ptr> 1 "address_mpx_no_index_operand" "Ti")
+			  (match_operand:<bnd_ptr> 2 "register_operand" "l")]
+			UNSPEC_BNDLDX_ADDR)])]
+		     UNSPEC_BNDLDX))
               (use (mem:BLK (match_dup 1)))])]
   "TARGET_MPX"
   "bndldx\t{%3, %0|%0, %3}"
Index: gcc/config/rx/rx.md
===================================================================
--- gcc/config/rx/rx.md	2015-05-08 14:42:57.823310127 +0100
+++ gcc/config/rx/rx.md	2015-05-08 14:43:12.515140307 +0100
@@ -1734,9 +1734,9 @@ (define_peephole2
 					 (match_dup 2)))
 	      (clobber (reg:CC CC_REG))])]
   "peep2_regno_dead_p (2, REGNO (operands[0])) && (optimize < 3 || optimize_size)"
-  [(parallel [(set:SI (match_dup 2)
-		      (memex_commutative:SI (match_dup 2)
-					    (extend_types:SI (match_dup 1))))
+  [(parallel [(set (match_dup 2)
+		   (memex_commutative:SI (match_dup 2)
+					 (extend_types:SI (match_dup 1))))
 	      (clobber (reg:CC CC_REG))])]
 )
 
@@ -1748,9 +1748,9 @@ (define_peephole2
 					 (match_dup 0)))
 	      (clobber (reg:CC CC_REG))])]
   "peep2_regno_dead_p (2, REGNO (operands[0])) && (optimize < 3 || optimize_size)"
-  [(parallel [(set:SI (match_dup 2)
-		      (memex_commutative:SI (match_dup 2)
-					    (extend_types:SI (match_dup 1))))
+  [(parallel [(set (match_dup 2)
+		   (memex_commutative:SI (match_dup 2)
+					 (extend_types:SI (match_dup 1))))
 	      (clobber (reg:CC CC_REG))])]
 )
 
@@ -1762,9 +1762,9 @@ (define_peephole2
 				     (match_dup 0)))
 	      (clobber (reg:CC CC_REG))])]
   "peep2_regno_dead_p (2, REGNO (operands[0])) && (optimize < 3 || optimize_size)"
-  [(parallel [(set:SI (match_dup 2)
-		      (memex_noncomm:SI (match_dup 2)
-					(extend_types:SI (match_dup 1))))
+  [(parallel [(set (match_dup 2)
+		   (memex_noncomm:SI (match_dup 2)
+				     (extend_types:SI (match_dup 1))))
 	      (clobber (reg:CC CC_REG))])]
 )
 
@@ -1775,9 +1775,9 @@ (define_peephole2
 	(memex_nocc:SI (match_dup 0)
 		       (match_dup 2)))]
   "peep2_regno_dead_p (2, REGNO (operands[0])) && (optimize < 3 || optimize_size)"
-  [(set:SI (match_dup 2)
-	   (memex_nocc:SI (match_dup 2)
-			  (extend_types:SI (match_dup 1))))]
+  [(set (match_dup 2)
+	(memex_nocc:SI (match_dup 2)
+		       (extend_types:SI (match_dup 1))))]
 )
 
 (define_peephole2
@@ -1787,9 +1787,9 @@ (define_peephole2
 	(memex_nocc:SI (match_dup 2)
 		       (match_dup 0)))]
   "peep2_regno_dead_p (2, REGNO (operands[0])) && (optimize < 3 || optimize_size)"
-  [(set:SI (match_dup 2)
-	   (memex_nocc:SI (match_dup 2)
-			  (extend_types:SI (match_dup 1))))]
+  [(set (match_dup 2)
+	(memex_nocc:SI (match_dup 2)
+		       (extend_types:SI (match_dup 1))))]
 )
 
 (define_insn "<memex_commutative:code>si3_<extend_types:code><small_int_modes:mode>"
@@ -2623,8 +2623,8 @@ (define_expand "pid_addr"
 )
 
 (define_insn "movdi"
-  [(set:DI (match_operand:DI 0 "nonimmediate_operand" "=rm")
-	   (match_operand:DI 1 "general_operand"      "rmi"))]
+  [(set (match_operand:DI 0 "nonimmediate_operand" "=rm")
+        (match_operand:DI 1 "general_operand"      "rmi"))]
   "TARGET_ENABLE_LRA"
   { return rx_gen_move_template (operands, false); }
   [(set_attr "length" "16")
@@ -2632,8 +2632,8 @@ (define_insn "movdi"
 )
 
 (define_insn "movdf"
-  [(set:DF (match_operand:DF 0 "nonimmediate_operand" "=rm")
-	   (match_operand:DF 1 "general_operand"      "rmi"))]
+  [(set (match_operand:DF 0 "nonimmediate_operand" "=rm")
+        (match_operand:DF 1 "general_operand"      "rmi"))]
   "TARGET_ENABLE_LRA"
   { return rx_gen_move_template (operands, false); }
   [(set_attr "length" "16")

Re: Remove mode argument from gen_rtx_SET

[DJ Delorie]

> ; This pattern is identical to the truncsipsi2 pattern except
> ; that it uses a SUBREG instead of a TRUNC.  It is needed in
> ; order to prevent reload from converting (set:SI (SUBREG:PSI (SI)))
> ; into (SET:PSI (PSI)).
> 
> I'm not sure what that's supposed to mean (what's an SI set of a PSI
> subreg?), but I suspect removing the mode would lose information,
> so I left it alone.

MSP430 has 20-bit registers (PSImode-sized).  One register can hold an
HI or PSI sized value, but if you have an SI value it's stored as two
HI registers.

Thus, a PSImode value in a register is *not* just the 20 LSB of an
SImode value.  Also, a PSImode subset of an SI value is stored
different than a PSImode value on its own.

Thus, consider code like this:

(set (reg:SI 1)
     (subreg:PSI (reg:SI 2)))

(set (reg:PSI 1)
     (reg:PSI 2))

On most architectures, you'd say "these do the same thing" but on
MSP430 they don't.

Re: Remove mode argument from gen_rtx_SET

[Richard Sandiford]

DJ Delorie <dj@redhat.com> writes:
>> ; This pattern is identical to the truncsipsi2 pattern except
>> ; that it uses a SUBREG instead of a TRUNC.  It is needed in
>> ; order to prevent reload from converting (set:SI (SUBREG:PSI (SI)))
>> ; into (SET:PSI (PSI)).
>> 
>> I'm not sure what that's supposed to mean (what's an SI set of a PSI
>> subreg?), but I suspect removing the mode would lose information,
>> so I left it alone.
>
> MSP430 has 20-bit registers (PSImode-sized).  One register can hold an
> HI or PSI sized value, but if you have an SI value it's stored as two
> HI registers.
>
> Thus, a PSImode value in a register is *not* just the 20 LSB of an
> SImode value.  Also, a PSImode subset of an SI value is stored
> different than a PSImode value on its own.
>
> Thus, consider code like this:
>
> (set (reg:SI 1)
>      (subreg:PSI (reg:SI 2)))
>
> (set (reg:PSI 1)
>      (reg:PSI 2))
>
> On most architectures, you'd say "these do the same thing" but on
> MSP430 they don't.

What I was confused about is that the first set isn't valid rtl.
The SET_SRC and SET_DEST always have to have the same mode
(or VOIDmode in the case of a CONST_INT, etc., where the mode
is implicitly the same as the SET_DEST).  So wouldn't it have
to be:

  (set (reg:SI 1)
       (subreg:SI (reg:PSI 2)))

or:

  (set (reg:PSI 1)
       (subreg:PSI (reg:SI 2)))

?

Thanks,
Richard

Re: Remove mode argument from gen_rtx_SET

[DJ Delorie]

> What I was confused about is that the first set isn't valid rtl.
> The SET_SRC and SET_DEST always have to have the same mode
> (or VOIDmode in the case of a CONST_INT, etc., where the mode
> is implicitly the same as the SET_DEST).  So wouldn't it have
> to be:
> 
>   (set (reg:SI 1)
>        (subreg:SI (reg:PSI 2)))
> 
> or:
> 
>   (set (reg:PSI 1)
>        (subreg:PSI (reg:SI 2)))
> 
> ?

If my memory doesn't match reality, I blame my memory.

I'd have to experiment a while to dig out the details, but the key
point is that a PSI in a reg is not stored the same as a PSI subreg of
an SI reg.  You have to keep that information across subregs or you
lose track of where the actual bits are.