| Message ID | 87egmsv677.fsf@e105548-lin.cambridge.arm.com |
|---|---|
| State | New |
| Headers | show |
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
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 */