Re: [PATCH, ARM, ping] Clean up arm backend using the @ construct for MD patterns

Fri, 23 Nov 2018 00:06:59 -0800 

Ping?

Best regards,

Mihail

On 11/07/2018 05:46 PM, Mihail Ionescu wrote:



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.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




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.ione...@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>),
     (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.
     * config/arm/sync.md:
     (atomic_compare_and_swap<CCSI:arch><NARROW:mode>_1),
     (atomic_compare_and_swap<CCSI:arch><SIDI:mode>_1): Likewise.

If everything is ok for trunk, can someone commit it on my behalf?

Best regards,
Mihail

Reply via email to