This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, ARM] Clean up arm backend using the @ construct for MD patterns
- From: Ramana Radhakrishnan <Ramana dot Radhakrishnan at arm dot com>
- To: Mihail Ionescu <mihail dot ionescu at foss dot arm dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>, Kyrylo Tkachov <Kyrylo dot Tkachov at arm dot com>
- Cc: nd <nd at arm dot com>
- Date: Tue, 9 Oct 2018 08:52:07 +0000
- Subject: Re: [PATCH, ARM] Clean up arm backend using the @ construct for MD patterns
- References: <d350c53f-f25e-0650-acaa-95ce28319820@foss.arm.com>
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.ionescu@arm.com><mailto:mihail.ionescu@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