* [PATCH] RISC-V: Fix opcode entries of "vmsge{,u}.vx"
@ 2023-08-06 1:53 Tsukasa OI
2023-08-08 6:42 ` Tsukasa OI
2023-08-11 3:41 ` Nelson Chu
0 siblings, 2 replies; 3+ messages in thread
From: Tsukasa OI @ 2023-08-06 1:53 UTC (permalink / raw)
To: Tsukasa OI, Palmer Dabbelt, Andrew Waterman, Jim Wilson,
Nelson Chu, Kito Cheng
Cc: binutils
From: Tsukasa OI <research_trasio@irq.a4lg.com>
Their check_func should be "match_never", not "match_opcode". The reasons
this error did not cause any disassembler errors are:
1. The problem will not reproduce if "no-aliases" is specified
(because macro instructions are handled as aliases).
2. If not, all affected compressed instructions or their aliases
precede before "vmsge{,u}.vx" macro instructions.
However, it'll easily break if we reorder opcode entries. This commit
fixes this issue before the *accident* occurs.
opcodes/ChangeLog:
* riscv-opc.c (riscv_opcodes): Make sure that we never match to
vmsge{,u}.vx instructions unless specified in the assembler.
---
opcodes/riscv-opc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
index 6a854736fec0..37c7694999ad 100644
--- a/opcodes/riscv-opc.c
+++ b/opcodes/riscv-opc.c
@@ -1606,10 +1606,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_opcode, INSN_MACRO },
-{"vmsge.vx", 0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE, match_opcode, INSN_MACRO },
-{"vmsgeu.vx", 0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU, match_opcode, INSN_MACRO },
-{"vmsgeu.vx", 0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU, match_opcode, 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", 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 },
{"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},
base-commit: 5e66f55c62e306afbcc93856bf06e542ddd00997
--
2.41.0
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] RISC-V: Fix opcode entries of "vmsge{,u}.vx"
2023-08-06 1:53 [PATCH] RISC-V: Fix opcode entries of "vmsge{,u}.vx" Tsukasa OI
@ 2023-08-08 6:42 ` Tsukasa OI
2023-08-11 3:41 ` Nelson Chu
1 sibling, 0 replies; 3+ messages in thread
From: Tsukasa OI @ 2023-08-08 6:42 UTC (permalink / raw)
To: Palmer Dabbelt, Andrew Waterman, Jim Wilson, Nelson Chu,
Kito Cheng, Jiawei
Cc: binutils
A little more about how important this fix is.
vmsge{,u}.vx matches wide range of compressed instructions and possibly
affect wide range of compressed extensions.
- xxxx xxxx xxx0 00x0 (vmsge.vx; M_VMSGE == 29)
- xxxx xxxx xxx0 000x (vmsgeu.vx; M_VMSGEU == 30)
The reason why we don't see any wrong disassembly is detailed in the
patch message. And I found that the 'Zcmp' extension patch (a work by
Jiawei) actually breaks the disassembler. This is not Jiawei's fault
but the consequence of this bug (I want to fix).
How to reproduce:
1. Apply and optionally fix Jiawei's Zcmp patchset
(applying only push/pop patch is sufficient to reproduce)
<https://sourceware.org/pipermail/binutils/2023-July/128626.html>
2. Assemble "cm.push {ra}, -16"
with -march=rv32if_zcmp_zve32f
3. Disassemble the output
and we see "vmsge.vx v16,v0,ra,v0.t", not "cm.push {ra}, -16".
This is because "cm.push {ra}, -16" mismatches as follows:
- 1011 1000 0100 0010 (cm.push {ra}, -16)
- xxxx xxxx xxx0 00x0 (wrong matching pattern of vmsge.vx)
Note that "cm.push" entry on riscv_opcodes is placed after "vmsge.vx".
I don't recommend Jiawei to move "cm.push" riscv_opcode entry because
there's no good reason to do it.
I recognize that fixing this bug is practically a requirement of
adopting the 'Zcmp' / 'Zcmt' extensions (not absolute requirement
though) and hope that this bug fix patch is reviewed soon.
Sincerely,
Tsukasa
On 2023/08/06 10:53, Tsukasa OI wrote:
> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>
> Their check_func should be "match_never", not "match_opcode". The reasons
> this error did not cause any disassembler errors are:
>
> 1. The problem will not reproduce if "no-aliases" is specified
> (because macro instructions are handled as aliases).
> 2. If not, all affected compressed instructions or their aliases
> precede before "vmsge{,u}.vx" macro instructions.
>
> However, it'll easily break if we reorder opcode entries. This commit
> fixes this issue before the *accident* occurs.
>
> opcodes/ChangeLog:
>
> * riscv-opc.c (riscv_opcodes): Make sure that we never match to
> vmsge{,u}.vx instructions unless specified in the assembler.
> ---
> opcodes/riscv-opc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
> index 6a854736fec0..37c7694999ad 100644
> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -1606,10 +1606,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_opcode, INSN_MACRO },
> -{"vmsge.vx", 0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE, match_opcode, INSN_MACRO },
> -{"vmsgeu.vx", 0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU, match_opcode, INSN_MACRO },
> -{"vmsgeu.vx", 0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU, match_opcode, 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", 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 },
>
> {"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},
>
> base-commit: 5e66f55c62e306afbcc93856bf06e542ddd00997
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] RISC-V: Fix opcode entries of "vmsge{,u}.vx"
2023-08-06 1:53 [PATCH] RISC-V: Fix opcode entries of "vmsge{,u}.vx" Tsukasa OI
2023-08-08 6:42 ` Tsukasa OI
@ 2023-08-11 3:41 ` Nelson Chu
1 sibling, 0 replies; 3+ messages in thread
From: Nelson Chu @ 2023-08-11 3:41 UTC (permalink / raw)
To: Tsukasa OI
Cc: Palmer Dabbelt, Andrew Waterman, Jim Wilson, Kito Cheng, binutils
[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]
Okay, in general INSN_MACRO should use match_never rather than match_opcode
and others, so looks correct and good to me.
Thanks
Nelson
On Sun, Aug 6, 2023 at 9:53 AM Tsukasa OI <research_trasio@irq.a4lg.com>
wrote:
> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>
> Their check_func should be "match_never", not "match_opcode". The reasons
> this error did not cause any disassembler errors are:
>
> 1. The problem will not reproduce if "no-aliases" is specified
> (because macro instructions are handled as aliases).
> 2. If not, all affected compressed instructions or their aliases
> precede before "vmsge{,u}.vx" macro instructions.
>
> However, it'll easily break if we reorder opcode entries. This commit
> fixes this issue before the *accident* occurs.
>
> opcodes/ChangeLog:
>
> * riscv-opc.c (riscv_opcodes): Make sure that we never match to
> vmsge{,u}.vx instructions unless specified in the assembler.
> ---
> opcodes/riscv-opc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/opcodes/riscv-opc.c b/opcodes/riscv-opc.c
> index 6a854736fec0..37c7694999ad 100644
> --- a/opcodes/riscv-opc.c
> +++ b/opcodes/riscv-opc.c
> @@ -1606,10 +1606,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_opcode, INSN_MACRO },
> -{"vmsge.vx", 0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGE,
> match_opcode, INSN_MACRO },
> -{"vmsgeu.vx", 0, INSN_CLASS_V, "Vd,Vt,sVm", 0, (int) M_VMSGEU,
> match_opcode, INSN_MACRO },
> -{"vmsgeu.vx", 0, INSN_CLASS_V, "Vd,Vt,s,VM,VT", 0, (int) M_VMSGEU,
> match_opcode, 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", 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 },
>
> {"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},
>
> base-commit: 5e66f55c62e306afbcc93856bf06e542ddd00997
> --
> 2.41.0
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-08-11 3:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-06 1:53 [PATCH] RISC-V: Fix opcode entries of "vmsge{,u}.vx" Tsukasa OI
2023-08-08 6:42 ` Tsukasa OI
2023-08-11 3:41 ` 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).