On 10/09/2018 09:52 AM, Ramana Radhakrishnan wrote:
> 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
>
Hi,
I have applied the requested changes, but left it as one patch.
Bootstrapped and tested it on arm-none-linux-gnueabihf and also ran
regressions on arm-none-eabi successfully.
gcc/ChangeLog:
2018-10-03 Mihail Ionescu <mihail.ionescu@arm.com>
* config/arm/arm.c (arm_expand_compare_and_swap): Simplify and call
gen_atomic_compare_swap_1.
(arm_evpc_neon_vuzp): Likewise gen_neon_vuzp_internal.
(arm_evpc_neon_vtrn): Likewise gen_neon_vtrn_internal.
(arm_evpc_neon_vext): Likewise gen_neon_vext_internal.
(arm_evpc_neon_vzip): Likewise gen_neon_vzip_internal.
(arm_evpc_neon_vrev): Replaced the function pointer and simplified the mode
checks.
* config/arm/arm.md (neon_vext<mode>),