On 09/10/2018 09:27, Mihail Ionescu wrote: > Hi all, > > This patch removes some of the machine mode checks from the arm backend when > emitting instructions by using the '@' construct (Parameterized Names[2]). It > is based on the previous AArch64 patch[1]. > > [1]https://gcc.gnu.org/ml/gcc-patches/2018-07/msg00673.html > [2]https://gcc.gnu.org/onlinedocs/gccint/Parameterized-Names.html#Parameterized-Names > > Ran the tests on arm-none-eabi.
Thanks for the patch - It would be good to split this into 2 patches, one for the cleanup with the permute instructions and the other for the atomics. That makes life easier with reviews and it's logically grouped as well. Testing on just arm-none-eabi for the atomic_compare_and_swap changes are not sufficient. I would prefer a bootstrap and test run on arm-none-linux-gnueabihf with (--with-arch=armv7-a --with-fpu=vfpv3-d16 --with-float=hard as your configure options). Alternatively I'd be happy if you could ensure that the libraries built for arm-none-eabi show no difference in code generation for your change ? That will give us some more confidence that nothing else is wrong here. > > > gcc/ChangeLog: > 2018-10-03 Mihail > Ionescu<mihail.ione...@arm.com><mailto:mihail.ione...@arm.com> > > * config/arm/arm.c (arm_expand_compare_and_swap): Use > gen_atomic_compare_and_swap_1 > instead of explicit mode checks. Simplify and call gen_atomic_compare_swap_1. > (arm_evpc_neon_vuzp): Likewise gen_neon_vuzp_internal. Simplify and call gen_neon_vuzp_internal.. > (arm_evpc_neon_vtrn): Likewise gen_neon_vtrn_internal. > (arm_evpc_neon_vext): Likewise gen_neon_vext. > (arm_evpc_neon_vzip): Likewise gen_neon_vzip_internal. > (arm_evpc_neon_vrev): Replaced the function pointer and simplified > the mode > checks. and so on... > * config/arm/arm.md (neon_vext<mode>) > (neon_vrev64<mode>, neon_vrev32<mode>) > (neon_vrev16<mode>, neon_vtrn<mode>_internal) > (neon_vzip<mode>_internal, neon_vuzp<mode>_internal): Add an > '@'character > before the pattern name. Separate all pattern names across lines with ,'s . > * config/arm/sync.md: > (atomic_compare_and_swap<CCSI:arch><NARROW:mode>_1) > (atomic_compare_and_swap<CCSI:arch><SIDI:mode>_1): Likewise. Same as above. > > If everything is ok for trunk, can someone commit it on my behalf? > > Best regards, > Mihail > > > diff.txt > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > index > 8810df53aa34798b5e3e1eb3a870101d530702e4..51441efa934f5f2a5963750fcd7e077951406d5a > 100644 > --- a/gcc/config/arm/arm.c > +++ b/gcc/config/arm/arm.c > @@ -28539,8 +28539,7 @@ void > arm_expand_compare_and_swap (rtx operands[]) > { > rtx bval, bdst, rval, mem, oldval, newval, is_weak, mod_s, mod_f, x; > - machine_mode mode; > - rtx (*gen) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx); > + machine_mode mode, arch_mode; s/arch_mode/compare_mode The mode here is the mode for the comparison , whether we use SImode as for Thumb1 or a CC_Zmode in !TARGET_THUMB1 cases. arch_mode doesn't tell me much on reading the name. > > bval = operands[0]; > rval = operands[1]; > @@ -28588,32 +28587,13 @@ arm_expand_compare_and_swap (rtx operands[]) > } > > if (TARGET_THUMB1) > - { > - switch (mode) > - { > - case E_QImode: gen = gen_atomic_compare_and_swapt1qi_1; break; > - case E_HImode: gen = gen_atomic_compare_and_swapt1hi_1; break; > - case E_SImode: gen = gen_atomic_compare_and_swapt1si_1; break; > - case E_DImode: gen = gen_atomic_compare_and_swapt1di_1; break; > - default: > - gcc_unreachable (); > - } > - } > + arch_mode = E_SImode; > else > - { > - switch (mode) > - { > - case E_QImode: gen = gen_atomic_compare_and_swap32qi_1; break; > - case E_HImode: gen = gen_atomic_compare_and_swap32hi_1; break; > - case E_SImode: gen = gen_atomic_compare_and_swap32si_1; break; > - case E_DImode: gen = gen_atomic_compare_and_swap32di_1; break; > - default: > - gcc_unreachable (); > - } > - } > + arch_mode = CC_Zmode; > > bdst = TARGET_THUMB1 ? bval : gen_rtx_REG (CC_Zmode, CC_REGNUM); > - emit_insn (gen (bdst, rval, mem, oldval, newval, is_weak, mod_s, mod_f)); > + emit_insn (gen_atomic_compare_and_swap_1 (arch_mode, mode, bdst, rval, > mem, oldval, > + newval, is_weak, mod_s, mod_f)); > > if (mode == QImode || mode == HImode) > emit_move_insn (operands[1], gen_lowpart (mode, rval)); > @@ -28979,7 +28959,6 @@ arm_evpc_neon_vuzp (struct expand_vec_perm_d *d) > { > unsigned int i, odd, mask, nelt = d->perm.length (); > rtx out0, out1, in0, in1; > - rtx (*gen)(rtx, rtx, rtx, rtx); > int first_elem; > int swap_nelt; > > @@ -29013,22 +28992,6 @@ arm_evpc_neon_vuzp (struct expand_vec_perm_d *d) > if (d->testing_p) > return true; > > - switch (d->vmode) > - { > - case E_V16QImode: gen = gen_neon_vuzpv16qi_internal; break; > - case E_V8QImode: gen = gen_neon_vuzpv8qi_internal; break; > - case E_V8HImode: gen = gen_neon_vuzpv8hi_internal; break; > - case E_V4HImode: gen = gen_neon_vuzpv4hi_internal; break; > - case E_V8HFmode: gen = gen_neon_vuzpv8hf_internal; break; > - case E_V4HFmode: gen = gen_neon_vuzpv4hf_internal; break; > - case E_V4SImode: gen = gen_neon_vuzpv4si_internal; break; > - case E_V2SImode: gen = gen_neon_vuzpv2si_internal; break; > - case E_V2SFmode: gen = gen_neon_vuzpv2sf_internal; break; > - case E_V4SFmode: gen = gen_neon_vuzpv4sf_internal; break; > - default: > - gcc_unreachable (); > - } > - > in0 = d->op0; > in1 = d->op1; > if (swap_nelt != 0) > @@ -29039,7 +29002,7 @@ arm_evpc_neon_vuzp (struct expand_vec_perm_d *d) > if (odd) > std::swap (out0, out1); > > - emit_insn (gen (out0, in0, in1, out1)); > + emit_insn (gen_neon_vuzp_internal (d->vmode, out0, in0, in1, out1)); > return true; > } > > @@ -29050,7 +29013,6 @@ arm_evpc_neon_vzip (struct expand_vec_perm_d *d) > { > unsigned int i, high, mask, nelt = d->perm.length (); > rtx out0, out1, in0, in1; > - rtx (*gen)(rtx, rtx, rtx, rtx); > int first_elem; > bool is_swapped; > > @@ -29088,22 +29050,6 @@ arm_evpc_neon_vzip (struct expand_vec_perm_d *d) > if (d->testing_p) > return true; > > - switch (d->vmode) > - { > - case E_V16QImode: gen = gen_neon_vzipv16qi_internal; break; > - case E_V8QImode: gen = gen_neon_vzipv8qi_internal; break; > - case E_V8HImode: gen = gen_neon_vzipv8hi_internal; break; > - case E_V4HImode: gen = gen_neon_vzipv4hi_internal; break; > - case E_V8HFmode: gen = gen_neon_vzipv8hf_internal; break; > - case E_V4HFmode: gen = gen_neon_vzipv4hf_internal; break; > - case E_V4SImode: gen = gen_neon_vzipv4si_internal; break; > - case E_V2SImode: gen = gen_neon_vzipv2si_internal; break; > - case E_V2SFmode: gen = gen_neon_vzipv2sf_internal; break; > - case E_V4SFmode: gen = gen_neon_vzipv4sf_internal; break; > - default: > - gcc_unreachable (); > - } > - > in0 = d->op0; > in1 = d->op1; > if (is_swapped) > @@ -29114,17 +29060,16 @@ arm_evpc_neon_vzip (struct expand_vec_perm_d *d) > if (high) > std::swap (out0, out1); > > - emit_insn (gen (out0, in0, in1, out1)); > + emit_insn (gen_neon_vzip_internal (d->vmode, out0, in0, in1, out1)); > return true; > } > > /* Recognize patterns for the VREV insns. */ > - > static bool > arm_evpc_neon_vrev (struct expand_vec_perm_d *d) > { > unsigned int i, j, diff, nelt = d->perm.length (); > - rtx (*gen)(rtx, rtx); > + rtx (*gen) (machine_mode, rtx, rtx); > > if (!d->one_vector_p) > return false; > @@ -29133,23 +29078,29 @@ arm_evpc_neon_vrev (struct expand_vec_perm_d *d) > switch (diff) > { > case 7: > - switch (d->vmode) > - { > - case E_V16QImode: gen = gen_neon_vrev64v16qi; break; > - case E_V8QImode: gen = gen_neon_vrev64v8qi; break; > - default: > - return false; > - } > - break; > + switch (d->vmode) > + { > + case E_V16QImode: > + case E_V8QImode: > + gen = gen_neon_vrev64; > + break; > + default: > + return false; > + } > + break; > case 3: > - switch (d->vmode) > - { > - case E_V16QImode: gen = gen_neon_vrev32v16qi; break; > - case E_V8QImode: gen = gen_neon_vrev32v8qi; break; > - case E_V8HImode: gen = gen_neon_vrev64v8hi; break; > - case E_V4HImode: gen = gen_neon_vrev64v4hi; break; > - case E_V8HFmode: gen = gen_neon_vrev64v8hf; break; > - case E_V4HFmode: gen = gen_neon_vrev64v4hf; break; > + switch (d->vmode) > + { > + case E_V16QImode: > + case E_V8QImode: > + gen = gen_neon_vrev32; > + break; > + case E_V8HImode: > + case E_V4HImode: > + case E_V8HFmode: > + case E_V4HFmode: > + gen = gen_neon_vrev64; > + break; > default: > return false; > } > @@ -29157,15 +29108,21 @@ arm_evpc_neon_vrev (struct expand_vec_perm_d *d) > case 1: > switch (d->vmode) > { > - case E_V16QImode: gen = gen_neon_vrev16v16qi; break; > - case E_V8QImode: gen = gen_neon_vrev16v8qi; break; > - case E_V8HImode: gen = gen_neon_vrev32v8hi; break; > - case E_V4HImode: gen = gen_neon_vrev32v4hi; break; > - case E_V4SImode: gen = gen_neon_vrev64v4si; break; > - case E_V2SImode: gen = gen_neon_vrev64v2si; break; > - case E_V4SFmode: gen = gen_neon_vrev64v4sf; break; > - case E_V2SFmode: gen = gen_neon_vrev64v2sf; break; > - default: > + case E_V16QImode: > + case E_V8QImode: > + gen = gen_neon_vrev16; > + break; > + case E_V8HImode: > + case E_V4HImode: > + gen = gen_neon_vrev32; > + break; > + case E_V4SImode: > + case E_V2SImode: > + case E_V4SFmode: > + case E_V2SFmode: > + gen = gen_neon_vrev64; > + break; > + default: > return false; > } > break; > @@ -29190,7 +29147,7 @@ arm_evpc_neon_vrev (struct expand_vec_perm_d *d) > if (d->testing_p) > return true; > > - emit_insn (gen (d->target, d->op0)); > + emit_insn (gen (d->vmode, d->target, d->op0)); > return true; > } > > @@ -29201,7 +29158,6 @@ arm_evpc_neon_vtrn (struct expand_vec_perm_d *d) > { > unsigned int i, odd, mask, nelt = d->perm.length (); > rtx out0, out1, in0, in1; > - rtx (*gen)(rtx, rtx, rtx, rtx); > > if (GET_MODE_UNIT_SIZE (d->vmode) >= 8) > return false > @@ -29227,22 +29183,6 @@ arm_evpc_neon_vtrn (struct > expand_vec_perm_d *d) > if (d->testing_p) > return true; > > - switch (d->vmode) > - { > - case E_V16QImode: gen = gen_neon_vtrnv16qi_internal; break; > - case E_V8QImode: gen = gen_neon_vtrnv8qi_internal; break; > - case E_V8HImode: gen = gen_neon_vtrnv8hi_internal; break; > - case E_V4HImode: gen = gen_neon_vtrnv4hi_internal; break; > - case E_V8HFmode: gen = gen_neon_vtrnv8hf_internal; break; > - case E_V4HFmode: gen = gen_neon_vtrnv4hf_internal; break; > - case E_V4SImode: gen = gen_neon_vtrnv4si_internal; break; > - case E_V2SImode: gen = gen_neon_vtrnv2si_internal; break; > - case E_V2SFmode: gen = gen_neon_vtrnv2sf_internal; break; > - case E_V4SFmode: gen = gen_neon_vtrnv4sf_internal; break; > - default: > - gcc_unreachable (); > - } > - > in0 = d->op0; > in1 = d->op1; > if (BYTES_BIG_ENDIAN) > @@ -29256,7 +29196,7 @@ arm_evpc_neon_vtrn (struct expand_vec_perm_d *d) > if (odd) > std::swap (out0, out1); > > - emit_insn (gen (out0, in0, in1, out1)); > + emit_insn (gen_neon_vtrn_internal (d->vmode, out0, in0, in1, out1)); > return true; > } > > @@ -29266,7 +29206,6 @@ static bool > arm_evpc_neon_vext (struct expand_vec_perm_d *d) > { > unsigned int i, nelt = d->perm.length (); > - rtx (*gen) (rtx, rtx, rtx, rtx); > rtx offset; > > unsigned int location; > @@ -29302,29 +29241,16 @@ arm_evpc_neon_vext (struct expand_vec_perm_d *d) > > location = d->perm[0]; > > - switch (d->vmode) > - { > - case E_V16QImode: gen = gen_neon_vextv16qi; break; > - case E_V8QImode: gen = gen_neon_vextv8qi; break; > - case E_V4HImode: gen = gen_neon_vextv4hi; break; > - case E_V8HImode: gen = gen_neon_vextv8hi; break; > - case E_V2SImode: gen = gen_neon_vextv2si; break; > - case E_V4SImode: gen = gen_neon_vextv4si; break; > - case E_V4HFmode: gen = gen_neon_vextv4hf; break; > - case E_V8HFmode: gen = gen_neon_vextv8hf; break; > - case E_V2SFmode: gen = gen_neon_vextv2sf; break; > - case E_V4SFmode: gen = gen_neon_vextv4sf; break; > - case E_V2DImode: gen = gen_neon_vextv2di; break; > - default: > - return false; > - } > - > /* Success! */ > if (d->testing_p) > return true; > > offset = GEN_INT (location); > - emit_insn (gen (d->target, d->op0, d->op1, offset)); > + > + if(d->vmode == E_DImode) > + return false; > + > + emit_insn (gen_neon_vext (d->vmode, d->target, d->op0, d->op1, offset)); > return true; > } > > diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md > index > 5aeee4b08c168c5060d2156edfcba40cb25b5f6f..ee0d0cf415c423aa7601ac4c12b429165c11a578 > 100644 > --- a/gcc/config/arm/neon.md > +++ b/gcc/config/arm/neon.md > @@ -4790,7 +4790,7 @@ if (BYTES_BIG_ENDIAN) > DONE; > }) > > -(define_insn "neon_vext<mode>" > +(define_insn "@neon_vext<mode>" > [(set (match_operand:VDQX 0 "s_register_operand" "=w") > (unspec:VDQX [(match_operand:VDQX 1 "s_register_operand" "w") > (match_operand:VDQX 2 "s_register_operand" "w") > @@ -4804,7 +4804,8 @@ if (BYTES_BIG_ENDIAN) > [(set_attr "type" "neon_ext<q>")] > ) > > -(define_insn "neon_vrev64<mode>" > + Extra new line. Unneeded. > +(define_insn "@neon_vrev64<mode>" > [(set (match_operand:VDQ 0 "s_register_operand" "=w") > (unspec:VDQ [(match_operand:VDQ 1 "s_register_operand" "w")] > UNSPEC_VREV64))] > @@ -4813,7 +4814,7 @@ if (BYTES_BIG_ENDIAN) > [(set_attr "type" "neon_rev<q>")] > ) > > -(define_insn "neon_vrev32<mode>" > +(define_insn "@neon_vrev32<mode>" > [(set (match_operand:VX 0 "s_register_operand" "=w") > (unspec:VX [(match_operand:VX 1 "s_register_operand" "w")] > UNSPEC_VREV32))] > @@ -4822,7 +4823,7 @@ if (BYTES_BIG_ENDIAN) > [(set_attr "type" "neon_rev<q>")] > ) > > -(define_insn "neon_vrev16<mode>" > +(define_insn "@neon_vrev16<mode>" > [(set (match_operand:VE 0 "s_register_operand" "=w") > (unspec:VE [(match_operand:VE 1 "s_register_operand" "w")] > UNSPEC_VREV16))] > @@ -5278,7 +5279,7 @@ if (BYTES_BIG_ENDIAN) > [(set_attr "type" "neon_tbl4")] > ) > > -(define_expand "neon_vtrn<mode>_internal" > +(define_expand "@neon_vtrn<mode>_internal" > [(parallel > [(set (match_operand:VDQWH 0 "s_register_operand") > (unspec:VDQWH [(match_operand:VDQWH 1 "s_register_operand") > @@ -5304,7 +5305,7 @@ if (BYTES_BIG_ENDIAN) > [(set_attr "type" "neon_permute<q>")] > ) > > -(define_expand "neon_vzip<mode>_internal" > +(define_expand "@neon_vzip<mode>_internal" > [(parallel > [(set (match_operand:VDQWH 0 "s_register_operand") > (unspec:VDQWH [(match_operand:VDQWH 1 "s_register_operand") > @@ -5330,7 +5331,7 @@ if (BYTES_BIG_ENDIAN) > [(set_attr "type" "neon_zip<q>")] > ) > > -(define_expand "neon_vuzp<mode>_internal" > +(define_expand "@neon_vuzp<mode>_internal" > [(parallel > [(set (match_operand:VDQWH 0 "s_register_operand") > (unspec:VDQWH [(match_operand:VDQWH 1 "s_register_operand") > diff --git a/gcc/config/arm/sync.md b/gcc/config/arm/sync.md > index > 08fc5d1a8ab4a07cd8aa2f3c9826af8e3326f9b1..1c3cd60cc4379cfc93cb178ac2a6a67caa0edf40 > 100644 > --- a/gcc/config/arm/sync.md > +++ b/gcc/config/arm/sync.md > @@ -186,7 +186,7 @@ > > ;; Constraints of this pattern must be at least as strict as those of the > ;; cbranchsi operations in thumb1.md and aim to be as permissive. > -(define_insn_and_split "atomic_compare_and_swap<CCSI:arch><NARROW:mode>_1" > +(define_insn_and_split "@atomic_compare_and_swap<CCSI:arch><NARROW:mode>_1" > [(set (match_operand:CCSI 0 "cc_register_operand" "=&c,&l,&l,&l") ;; bool > out > (unspec_volatile:CCSI [(const_int 0)] VUNSPEC_ATOMIC_CAS)) > (set (match_operand:SI 1 "s_register_operand" "=&r,&l,&0,&l*h") ;; val > out > @@ -218,7 +218,7 @@ > > ;; Constraints of this pattern must be at least as strict as those of the > ;; cbranchsi operations in thumb1.md and aim to be as permissive. > -(define_insn_and_split "atomic_compare_and_swap<CCSI:arch><SIDI:mode>_1" > +(define_insn_and_split "@atomic_compare_and_swap<CCSI:arch><SIDI:mode>_1" > [(set (match_operand:CCSI 0 "cc_register_operand" "=&c,&l,&l,&l") ;; bool > out > (unspec_volatile:CCSI [(const_int 0)] VUNSPEC_ATOMIC_CAS)) > (set (match_operand:SIDI 1 "s_register_operand" "=&r,&l,&0,&l*h") > ;; val out > Otherwise OK if no regressions or if you satisfy the conditions above. Thanks, Ramana
