Re: Remove mode argument from gen_rtx_SET
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.
Re: Remove mode argument from gen_rtx_SET
DJ Delorie d...@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
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,
Re: Remove mode argument from gen_rtx_SET
Franz Sirl franz.sirl-ker...@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)
+
Re: Remove mode argument from gen_rtx_SET
; 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
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. 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
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
Remove mode argument from gen_rtx_SET
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,
Re: Remove mode argument from gen_rtx_SET
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
Jeff Law l...@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)
Re: Remove mode argument from gen_rtx_SET
On 05/07/2015 07:59 AM, Richard Sandiford wrote:
Jeff Law l...@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
