RE: [PATCH]AArch64 Rewrite simd move immediate patterns to new syntax

[Tamar Christina <Tamar.Christina@arm.com>]

> -----Original Message-----
> From: Ramana Radhakrishnan <ramana.gcc@googlemail.com>
> Sent: Wednesday, September 27, 2023 2:28 AM
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw
> <Richard.Earnshaw@arm.com>; Marcus Shawcroft
> <Marcus.Shawcroft@arm.com>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>;
> Richard Sandiford <Richard.Sandiford@arm.com>
> Subject: Re: [PATCH]AArch64 Rewrite simd move immediate patterns to new
> syntax
> 
> On Wed, Sep 27, 2023 at 1:53 AM Tamar Christina
> <tamar.christina@arm.com> wrote:
> >
> > Hi All,
> >
> > This rewrites the simd MOV patterns to use the new compact syntax.
> > No change in semantics is expected.  This will be needed in follow on patches.
> >
> > This also merges the splits into the define_insn which will also be needed
> soon.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> >
> > Ok for master?
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> >         PR tree-optimization/109154
> >         * config/aarch64/aarch64-simd.md
> (*aarch64_simd_mov<VDMOV:mode>):
> >         Rewrite to new syntax.
> >         (*aarch64_simd_mov<VQMOV:mode): Rewrite to new syntax and
> merge in
> >         splits.
> >
> > --- inline copy of patch --
> > diff --git a/gcc/config/aarch64/aarch64-simd.md
> > b/gcc/config/aarch64/aarch64-simd.md
> > index
> >
> e955691f1be8830efacc237465119764ce2a4942..7b4d5a37a9795fefda785
> aaacc24
> > 6918826ed0a2 100644
> > --- a/gcc/config/aarch64/aarch64-simd.md
> > +++ b/gcc/config/aarch64/aarch64-simd.md
> > @@ -143,54 +143,57 @@ (define_insn
> "aarch64_dup_lane_<vswap_width_name><mode>"
> >  )
> >
> >  (define_insn "*aarch64_simd_mov<VDMOV:mode>"
> > -  [(set (match_operand:VDMOV 0 "nonimmediate_operand"
> > -               "=w, r, m,  m, m,  w, ?r, ?w, ?r,  w,  w")
> > -       (match_operand:VDMOV 1 "general_operand"
> > -               "m,  m, Dz, w, r,  w,  w,  r,  r, Dn, Dz"))]
> > +  [(set (match_operand:VDMOV 0 "nonimmediate_operand")
> > +       (match_operand:VDMOV 1 "general_operand"))]
> >    "TARGET_FLOAT
> >     && (register_operand (operands[0], <MODE>mode)
> >         || aarch64_simd_reg_or_zero (operands[1], <MODE>mode))"
> > -  "@
> > -   ldr\t%d0, %1
> > -   ldr\t%x0, %1
> > -   str\txzr, %0
> > -   str\t%d1, %0
> > -   str\t%x1, %0
> > -   * return TARGET_SIMD ? \"mov\t%0.<Vbtype>, %1.<Vbtype>\" :
> \"fmov\t%d0, %d1\";
> > -   * return TARGET_SIMD ? \"umov\t%0, %1.d[0]\" : \"fmov\t%x0, %d1\";
> > -   fmov\t%d0, %1
> > -   mov\t%0, %1
> > -   * return aarch64_output_simd_mov_immediate (operands[1], 64);
> > -   fmov\t%d0, xzr"
> > -  [(set_attr "type" "neon_load1_1reg<q>, load_8, store_8,
> neon_store1_1reg<q>,\
> > -                    store_8, neon_logic<q>, neon_to_gp<q>, f_mcr,\
> > -                    mov_reg, neon_move<q>, f_mcr")
> > -   (set_attr "arch" "*,*,*,*,*,*,*,*,*,simd,*")]
> > -)
> > -
> > -(define_insn "*aarch64_simd_mov<VQMOV:mode>"
> > -  [(set (match_operand:VQMOV 0 "nonimmediate_operand"
> > -               "=w, Umn,  m,  w, ?r, ?w, ?r, w,  w")
> > -       (match_operand:VQMOV 1 "general_operand"
> > -               "m,  Dz, w,  w,  w,  r,  r, Dn, Dz"))]
> > +  {@ [cons: =0, 1; attrs: type, arch]
> > +     [w , m ; neon_load1_1reg<q> , *   ] ldr\t%d0, %1
> > +     [r , m ; load_8             , *   ] ldr\t%x0, %1
> > +     [m , Dz; store_8            , *   ] str\txzr, %0
> > +     [m , w ; neon_store1_1reg<q>, *   ] str\t%d1, %0
> > +     [m , r ; store_8            , *   ] str\t%x1, %0
> 
> This hunk starting here.
> 
> > +     [w , w ; neon_logic<q>      , simd] mov\t%0.<Vbtype>, %1.<Vbtype>
> > +     [w , w ; neon_logic<q>      , *   ] fmov\t%d0, %d1
> 
> Can you explain how this hunk is equivalent to
> 
> return TARGET_SIMD ? \"mov\t%0.<Vbtype>, %1.<Vbtype>\" : \"fmov\t%d0,
> %d1\"; ?
> 

LRA matches constraints from left to right, or with the new patterns from top to bottom.
So since both alternatives are w, w, their costs would always be the same.  In which case
LRA will pick the first matching one.  If SIMD is enabled that would be the first one, which
corresponds to TARGET_SIMD.  If it's not enabled the second alternative would be chosen.

In fact since w, w would have no losers the second alternative would not even be inspected
If the first one is enabled.  So the equivalence is maintained by the order of which alternatives
are processed.

> > +     [?r, w ; neon_to_gp<q>      , simd] umov\t%0, %1.d[0]
> > +     [?r, w ; neon_to_gp<q>      , *   ] fmov\t%x0, %d1
> > +     [?w, r ; f_mcr              , *   ] fmov\t%d0, %1
> > +     [?r, r ; mov_reg            , *   ] mov\t%0, %1
> > +     [w , Dn; neon_move<q>       , simd] <<
> aarch64_output_simd_mov_immediate (operands[1], 64);
> > +     [w , Dz; f_mcr              , *   ] fmov\t%d0, xzr
> > +  }
> > +)
> > +
> > +(define_insn_and_split "*aarch64_simd_mov<VQMOV:mode>"
> > +  [(set (match_operand:VQMOV 0 "nonimmediate_operand")
> > +       (match_operand:VQMOV 1 "general_operand"))]
> >    "TARGET_FLOAT
> >     && (register_operand (operands[0], <MODE>mode)
> >         || aarch64_simd_reg_or_zero (operands[1], <MODE>mode))"
> > -  "@
> > -   ldr\t%q0, %1
> > -   stp\txzr, xzr, %0
> > -   str\t%q1, %0
> > -   mov\t%0.<Vbtype>, %1.<Vbtype>
> > -   #
> > -   #
> > -   #
> > -   * return aarch64_output_simd_mov_immediate (operands[1], 128);
> > -   fmov\t%d0, xzr"
> > -  [(set_attr "type" "neon_load1_1reg<q>, store_16, neon_store1_1reg<q>,\
> > -                    neon_logic<q>, multiple, multiple,\
> > -                    multiple, neon_move<q>, fmov")
> > -   (set_attr "length" "4,4,4,4,8,8,8,4,4")
> > -   (set_attr "arch" "*,*,*,simd,*,*,*,simd,*")]
> > +  {@ [cons: =0, 1; attrs: type, arch, length]
> > +     [w  , m ; neon_load1_1reg<q> , *   , 4] ldr\t%q0, %1
> > +     [Umn, Dz; store_16           , *   , 4] stp\txzr, xzr, %0
> > +     [m  , w ; neon_store1_1reg<q>, *   , 4] str\t%q1, %0
> > +     [w  , w ; neon_logic<q>      , simd, 4] mov\t%0.<Vbtype>, %1.<Vbtype>
> > +     [?r , w ; multiple           , *   , 8] #
> > +     [?w , r ; multiple           , *   , 8] #
> > +     [?r , r ; multiple           , *   , 8] #
> > +     [w  , Dn; neon_move<q>       , simd, 4] <<
> aarch64_output_simd_mov_immediate (operands[1], 128);
> > +     [w  , Dz; fmov               , *   , 4] fmov\t%d0, xzr
> > +  }
> > +  "&& reload_completed
> > +   && !(FP_REGNUM_P (REGNO (operands[0]))
> > +       && FP_REGNUM_P (REGNO (operands[1])))"
> > +  [(const_int 0)]
> > +  {
> > +    if (GP_REGNUM_P (REGNO (operands[0]))
> > +       && GP_REGNUM_P (REGNO (operands[1])))
> > +      aarch64_simd_emit_reg_reg_move (operands, DImode, 2);
> > +    else
> > +      aarch64_split_simd_move (operands[0], operands[1]);
> > +    DONE;
> > +  }
> >  )
> >
> 
> 
> Reads correctly at first glance. Perhaps a sanity check with the
> aarch64 simd intrinsics suite, vect.exp or tsvc under a suitable multilib to give
> some confidence as to no code changes. ?

Hmm what multilib did you have in mind? AArch64 only has ILP32 as additional
multilibs.

Regards,
Tamar

> 
> Reviewed-by:   Ramana Radhakrishnan  <ramana@gcc.gnu.org>
> 
> regards
> Ramana
> 
> Ramana
> 
> 
> 
> 
> >  ;; When storing lane zero we can use the normal STR and its more
> > permissive @@ -276,33 +279,6 @@ (define_insn
> "vec_store_pair<VQ:mode><VQ2:mode>"
> >    [(set_attr "type" "neon_stp_q")]
> >  )
> >
> > -
> > -(define_split
> > -  [(set (match_operand:VQMOV 0 "register_operand" "")
> > -       (match_operand:VQMOV 1 "register_operand" ""))]
> > -  "TARGET_FLOAT
> > -   && reload_completed
> > -   && GP_REGNUM_P (REGNO (operands[0]))
> > -   && GP_REGNUM_P (REGNO (operands[1]))"
> > -  [(const_int 0)]
> > -{
> > -  aarch64_simd_emit_reg_reg_move (operands, DImode, 2);
> > -  DONE;
> > -})
> > -
> > -(define_split
> > -  [(set (match_operand:VQMOV 0 "register_operand" "")
> > -        (match_operand:VQMOV 1 "register_operand" ""))]
> > -  "TARGET_FLOAT
> > -   && reload_completed
> > -   && ((FP_REGNUM_P (REGNO (operands[0])) && GP_REGNUM_P (REGNO
> (operands[1])))
> > -       || (GP_REGNUM_P (REGNO (operands[0])) && FP_REGNUM_P
> (REGNO (operands[1]))))"
> > -  [(const_int 0)]
> > -{
> > -  aarch64_split_simd_move (operands[0], operands[1]);
> > -  DONE;
> > -})
> > -
> >  (define_expand "@aarch64_split_simd_mov<mode>"
> >    [(set (match_operand:VQMOV 0)
> >         (match_operand:VQMOV 1))]
> >
> >
> >
> >
> > --