diff mbox series

RISC-V: fold duplicate code in vector_macro()

Message ID 34e8123e-1278-4e0f-7713-1fcd2bff05eb@suse.com
State New
Headers
Series RISC-V: fold duplicate code in vector_macro() |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed

Commit Message

Jan Beulich Sept. 1, 2023, 12:34 p.m. UTC 
  There's no need to have almost identical code twice. Do away with
M_VMSGEU and instead simply use an unused (for these macros) field to
tell apart both variants.
---
While looking at the code, I was wondering about the usefulness of the
"mask" local variable ...

I also find "macro_build (..., "Vd,Vt,sVm", vd, vs2, vs1, vm);"
misleading: While the initial if() guarantees vm == 0 everywhere else
in the involved code, passing an apparently non-negative value into
macro_build() (when Vm expects either 0 or -1) is perhaps latently
problematic. Imo it would be better to either pass literal 0 there, or
to pass -vm. (Since the line is being touched anyway, I'd be happy to
make the adjustment right here.)

Comments

Nelson Chu Sept. 4, 2023, 2 a.m. UTC | #1 
Thanks!  LGTM :-)

Nelson

On Fri, Sep 1, 2023 at 8:34 PM Jan Beulich <jbeulich@suse.com> wrote:

> There's no need to have almost identical code twice. Do away with
> M_VMSGEU and instead simply use an unused (for these macros) field to
> tell apart both variants.
> ---
> While looking at the code, I was wondering about the usefulness of the
> "mask" local variable ...
>
> I also find "macro_build (..., "Vd,Vt,sVm", vd, vs2, vs1, vm);"
> misleading: While the initial if() guarantees vm == 0 everywhere else
> in the involved code, passing an apparently non-negative value into
> macro_build() (when Vm expects either 0 or -1) is perhaps latently
> problematic. Imo it would be better to either pass literal 0 there, or
> to pass -vm. (Since the line is being touched anyway, I'd be happy to
> make the adjustment right here.)
>
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1949,6 +1949,7 @@ vector_macro (struct riscv_cl_insn *ip)
>    int vs2 = (ip->insn_opcode >> OP_SH_VS2) & OP_MASK_VS2;
>    int vm = (ip->insn_opcode >> OP_SH_VMASK) & OP_MASK_VMASK;
>    int vtemp = (ip->insn_opcode >> OP_SH_VFUNCT6) & OP_MASK_VFUNCT6;
> +  const char *vmslt_vx = ip->insn_mo->match ? "vmsltu.vx" : "vmslt.vx";
>    int mask = ip->insn_mo->mask;
>
>    switch (mask)
> @@ -1957,7 +1958,7 @@ vector_macro (struct riscv_cl_insn *ip)
>        if (vm)
>         {
>           /* Unmasked.  */
> -         macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vd, vs2, vs1, -1);
> +         macro_build (NULL, vmslt_vx, "Vd,Vt,sVm", vd, vs2, vs1, -1);
>           macro_build (NULL, "vmnand.mm", "Vd,Vt,Vs", vd, vd, vd);
>           break;
>         }
> @@ -1966,13 +1967,13 @@ vector_macro (struct riscv_cl_insn *ip)
>           /* Masked.  Have vtemp to avoid overlap constraints.  */
>           if (vd == vm)
>             {
> -             macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp, vs2, vs1,
> -1);
> +             macro_build (NULL, vmslt_vx, "Vd,Vt,sVm", vtemp, vs2, vs1,
> -1);
>               macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vm,
> vtemp);
>             }
>           else
>             {
>               /* Preserve the value of vd if not updating by vm.  */
> -             macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp, vs2, vs1,
> -1);
> +             macro_build (NULL, vmslt_vx, "Vd,Vt,sVm", vtemp, vs2, vs1,
> -1);
>               macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vtemp, vm,
> vtemp);
>               macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vd, vm);
>               macro_build (NULL, "vmor.mm", "Vd,Vt,Vs", vd, vtemp, vd);
> @@ -1981,42 +1982,7 @@ vector_macro (struct riscv_cl_insn *ip)
>        else if (vd != vm)
>         {
>           /* Masked.  This may cause the vd overlaps vs2, when LMUL > 1.
> */
> -         macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vd, vs2, vs1, vm);
> -         macro_build (NULL, "vmxor.mm", "Vd,Vt,Vs", vd, vd, vm);
> -       }
> -      else
> -       as_bad (_("must provide temp if destination overlaps mask"));
> -      break;
> -
> -    case M_VMSGEU:
> -      if (vm)
> -       {
> -         /* Unmasked.  */
> -         macro_build (NULL, "vmsltu.vx", "Vd,Vt,sVm", vd, vs2, vs1, -1);
> -         macro_build (NULL, "vmnand.mm", "Vd,Vt,Vs", vd, vd, vd);
> -         break;
> -       }
> -      if (vtemp != 0)
> -       {
> -         /* Masked.  Have vtemp to avoid overlap constraints.  */
> -         if (vd == vm)
> -           {
> -             macro_build (NULL, "vmsltu.vx", "Vd,Vt,sVm", vtemp, vs2,
> vs1, -1);
> -             macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vm,
> vtemp);
> -           }
> -         else
> -           {
> -             /* Preserve the value of vd if not updating by vm.  */
> -             macro_build (NULL, "vmsltu.vx", "Vd,Vt,sVm", vtemp, vs2,
> vs1, -1);
> -             macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vtemp, vm,
> vtemp);
> -             macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vd, vm);
> -             macro_build (NULL, "vmor.mm", "Vd,Vt,Vs", vd, vtemp, vd);
> -           }
> -       }
> -      else if (vd != vm)
> -       {
> -         /* Masked.  This may cause the vd overlaps vs2, when LMUL > 1.
> */
> -         macro_build (NULL, "vmsltu.vx", "Vd,Vt,sVm", vd, vs2, vs1, vm);
> +         macro_build (NULL, vmslt_vx, "Vd,Vt,sVm", vd, vs2, vs1, vm);
>           macro_build (NULL, "vmxor.mm", "Vd,Vt,Vs", vd, vd, vm);
>         }
>        else
> @@ -2171,7 +2137,6 @@ macro (struct riscv_cl_insn *ip, express
>        break;
>
>      case M_VMSGE:
> -    case M_VMSGEU:
>        vector_macro (ip);
>        break;
>
> --- a/include/opcode/riscv.h
> +++ b/include/opcode/riscv.h
> @@ -563,7 +563,6 @@ enum
>    M_SEXTB,
>    M_SEXTH,
>    M_VMSGE,
> -  M_VMSGEU,
>    M_FLH,
>    M_FSH,
>    M_NUM_MACROS
> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -1618,10 +1618,10 @@ const struct riscv_opcode riscv_opcodes[
>  {"vmsgeu.vi",  0, INSN_CLASS_V,  "Vd,Vu,0Vm", MATCH_VMSEQVV,
> MASK_VMSEQVV, match_vs1_eq_vs2, INSN_ALIAS },
>  {"vmsgeu.vi",  0, INSN_CLASS_V,  "Vd,Vt,VkVm", MATCH_VMSGTUVI,
> MASK_VMSGTUVI, match_opcode, INSN_ALIAS },
>
> -{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGE,
> match_never, INSN_MACRO },
> -{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE,
> match_never, INSN_MACRO },
> -{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU,
> match_never, INSN_MACRO },
> -{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU,
> match_never, INSN_MACRO },
> +{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGE,
> match_never, INSN_MACRO },
> +{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE,
> match_never, INSN_MACRO },
> +{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,sVm", 1, (int) M_VMSGE,
> match_never, INSN_MACRO },
> +{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 1, (int) M_VMSGE,
> match_never, INSN_MACRO },
>
>  {"vminu.vv",   0, INSN_CLASS_V,  "Vd,Vt,VsVm", MATCH_VMINUVV,
> MASK_VMINUVV, match_opcode, 0},
>  {"vminu.vx",   0, INSN_CLASS_V,  "Vd,Vt,sVm", MATCH_VMINUVX,
> MASK_VMINUVX, match_opcode, 0},
>
diff mbox series

Patch

--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1949,6 +1949,7 @@  vector_macro (struct riscv_cl_insn *ip)
   int vs2 = (ip->insn_opcode >> OP_SH_VS2) & OP_MASK_VS2;
   int vm = (ip->insn_opcode >> OP_SH_VMASK) & OP_MASK_VMASK;
   int vtemp = (ip->insn_opcode >> OP_SH_VFUNCT6) & OP_MASK_VFUNCT6;
+  const char *vmslt_vx = ip->insn_mo->match ? "vmsltu.vx" : "vmslt.vx";
   int mask = ip->insn_mo->mask;
 
   switch (mask)
@@ -1957,7 +1958,7 @@  vector_macro (struct riscv_cl_insn *ip)
       if (vm)
 	{
 	  /* Unmasked.  */
-	  macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vd, vs2, vs1, -1);
+	  macro_build (NULL, vmslt_vx, "Vd,Vt,sVm", vd, vs2, vs1, -1);
 	  macro_build (NULL, "vmnand.mm", "Vd,Vt,Vs", vd, vd, vd);
 	  break;
 	}
@@ -1966,13 +1967,13 @@  vector_macro (struct riscv_cl_insn *ip)
 	  /* Masked.  Have vtemp to avoid overlap constraints.  */
 	  if (vd == vm)
 	    {
-	      macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp, vs2, vs1, -1);
+	      macro_build (NULL, vmslt_vx, "Vd,Vt,sVm", vtemp, vs2, vs1, -1);
 	      macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vm, vtemp);
 	    }
 	  else
 	    {
 	      /* Preserve the value of vd if not updating by vm.  */
-	      macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vtemp, vs2, vs1, -1);
+	      macro_build (NULL, vmslt_vx, "Vd,Vt,sVm", vtemp, vs2, vs1, -1);
 	      macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vtemp, vm, vtemp);
 	      macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vd, vm);
 	      macro_build (NULL, "vmor.mm", "Vd,Vt,Vs", vd, vtemp, vd);
@@ -1981,42 +1982,7 @@  vector_macro (struct riscv_cl_insn *ip)
       else if (vd != vm)
 	{
 	  /* Masked.  This may cause the vd overlaps vs2, when LMUL > 1.  */
-	  macro_build (NULL, "vmslt.vx", "Vd,Vt,sVm", vd, vs2, vs1, vm);
-	  macro_build (NULL, "vmxor.mm", "Vd,Vt,Vs", vd, vd, vm);
-	}
-      else
-	as_bad (_("must provide temp if destination overlaps mask"));
-      break;
-
-    case M_VMSGEU:
-      if (vm)
-	{
-	  /* Unmasked.  */
-	  macro_build (NULL, "vmsltu.vx", "Vd,Vt,sVm", vd, vs2, vs1, -1);
-	  macro_build (NULL, "vmnand.mm", "Vd,Vt,Vs", vd, vd, vd);
-	  break;
-	}
-      if (vtemp != 0)
-	{
-	  /* Masked.  Have vtemp to avoid overlap constraints.  */
-	  if (vd == vm)
-	    {
-	      macro_build (NULL, "vmsltu.vx", "Vd,Vt,sVm", vtemp, vs2, vs1, -1);
-	      macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vm, vtemp);
-	    }
-	  else
-	    {
-	      /* Preserve the value of vd if not updating by vm.  */
-	      macro_build (NULL, "vmsltu.vx", "Vd,Vt,sVm", vtemp, vs2, vs1, -1);
-	      macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vtemp, vm, vtemp);
-	      macro_build (NULL, "vmandnot.mm", "Vd,Vt,Vs", vd, vd, vm);
-	      macro_build (NULL, "vmor.mm", "Vd,Vt,Vs", vd, vtemp, vd);
-	    }
-	}
-      else if (vd != vm)
-	{
-	  /* Masked.  This may cause the vd overlaps vs2, when LMUL > 1.  */
-	  macro_build (NULL, "vmsltu.vx", "Vd,Vt,sVm", vd, vs2, vs1, vm);
+	  macro_build (NULL, vmslt_vx, "Vd,Vt,sVm", vd, vs2, vs1, vm);
 	  macro_build (NULL, "vmxor.mm", "Vd,Vt,Vs", vd, vd, vm);
 	}
       else
@@ -2171,7 +2137,6 @@  macro (struct riscv_cl_insn *ip, express
       break;
 
     case M_VMSGE:
-    case M_VMSGEU:
       vector_macro (ip);
       break;
 
--- a/include/opcode/riscv.h
+++ b/include/opcode/riscv.h
@@ -563,7 +563,6 @@  enum
   M_SEXTB,
   M_SEXTH,
   M_VMSGE,
-  M_VMSGEU,
   M_FLH,
   M_FSH,
   M_NUM_MACROS
--- a/opcodes/riscv-opc.c
+++ b/opcodes/riscv-opc.c
@@ -1618,10 +1618,10 @@  const struct riscv_opcode riscv_opcodes[
 {"vmsgeu.vi",  0, INSN_CLASS_V,  "Vd,Vu,0Vm", MATCH_VMSEQVV, MASK_VMSEQVV, match_vs1_eq_vs2, INSN_ALIAS },
 {"vmsgeu.vi",  0, INSN_CLASS_V,  "Vd,Vt,VkVm", MATCH_VMSGTUVI, MASK_VMSGTUVI, match_opcode, INSN_ALIAS },
 
-{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGE, match_never, INSN_MACRO },
-{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE, match_never, INSN_MACRO },
-{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU, match_never, INSN_MACRO },
-{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU, match_never, INSN_MACRO },
+{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGE, match_never, INSN_MACRO },
+{"vmsge.vx",   0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE, match_never, INSN_MACRO },
+{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,sVm", 1, (int) M_VMSGE, match_never, INSN_MACRO },
+{"vmsgeu.vx",  0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 1, (int) M_VMSGE, match_never, INSN_MACRO },
 
 {"vminu.vv",   0, INSN_CLASS_V,  "Vd,Vt,VsVm", MATCH_VMINUVV, MASK_VMINUVV, match_opcode, 0},
 {"vminu.vx",   0, INSN_CLASS_V,  "Vd,Vt,sVm", MATCH_VMINUVX, MASK_VMINUVX, match_opcode, 0},