* [PATCH] RISC-V: fold duplicate code in vector_macro()
@ 2023-09-01 12:34 Jan Beulich
2023-09-04 2:00 ` Nelson Chu
0 siblings, 1 reply; 2+ messages in thread
From: Jan Beulich @ 2023-09-01 12:34 UTC (permalink / raw)
To: Binutils; +Cc: Palmer Dabbelt, Andrew Waterman, Jim Wilson, Nelson Chu
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},
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] RISC-V: fold duplicate code in vector_macro()
2023-09-01 12:34 [PATCH] RISC-V: fold duplicate code in vector_macro() Jan Beulich
@ 2023-09-04 2:00 ` Nelson Chu
0 siblings, 0 replies; 2+ messages in thread
From: Nelson Chu @ 2023-09-04 2:00 UTC (permalink / raw)
To: Jan Beulich; +Cc: Binutils, Palmer Dabbelt, Andrew Waterman, Jim Wilson
[-- Attachment #1: Type: text/plain, Size: 6288 bytes --]
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},
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-09-04 2:00 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-01 12:34 [PATCH] RISC-V: fold duplicate code in vector_macro() Jan Beulich
2023-09-04 2:00 ` Nelson Chu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).