Add p-ext support with spec v0.94 #257

linsinan1995 · 2021-04-23T20:59:21Z

Add p-ext support with spec v0.93

Andes-tech and Shao-Chung Wang implemented the RISC-V 'p' extension with the spec version 0.51 with Binutils 2.32
Sinan Lin updated the implementation to the spec version 0.93 with Binutils 2.36

Nelson1225

I think you probably miss the porting from riscv-dis.c, since you add several of new operands, but you don't teach the dis-assembler to recognize these operands. Therefore, I suppose the gas test cases will be failed.

Nelson1225 · 2021-04-26T01:46:53Z

gas/config/tc-riscv.c

+#define MAX_KEYWORD_LEN 32
+
+static bfd_boolean
+parse_nds_v5_field (const char **str, char name[MAX_KEYWORD_LEN])


I suppose p should be a standard extension, rather than vendor extension? If my understanding is correct, then we should rename the operands without the vendor prefix.

‘nds’ was the prefix before spec 0.80. Since we are using the old prefix nds as a prefix so I keep the 'nds'. I will change it to rvp. Thanks.

Nelson1225 · 2021-04-26T01:55:06Z

gas/config/tc-riscv.c

@@ -1118,6 +1154,40 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
 	     return FALSE;
 	  }
 	break;
+      case 'n':


We should renamed the operands eventually. Rename to `p' prefix is more reasonable and easy to understand, just like what vector instructions did, but we already have p operand for another usage. Maybe we can file an issue to discuss the problem.

I checked 'p', 'P', 'r'(for rvp), but they are taken by others. so I used the old prefix 'nds'. It would be great to have a discussion on that. Thanks!

Nelson1225 · 2021-04-26T02:16:20Z

gas/config/tc-riscv.c

@@ -1083,6 +1118,7 @@ validate_riscv_insn (const struct riscv_opcode *opc, int length)
      case 'P': USE_BITS (OP_MASK_PRED, OP_SH_PRED); break;
      case 'Q': USE_BITS (OP_MASK_SUCC, OP_SH_SUCC); break;
      case 'o': /* ITYPE immediate, load displacement.  */
+      case 'l': /* IMM6L */


This looks strange. We encode it as ENCODE_ITYPE_IMM here (validate_riscv_insn), but will use ENCODE_SBTYPE_IMM6L to encode it later in the riscv_ip.

Thanks for pointing it out. Fixed it the latest commit.

Nelson1225 · 2021-04-26T02:17:53Z

gas/config/tc-riscv.c

+		      {
+			if (xlen == 32 && (regno % 2) != 0)
+			  {
+			    as_bad (_("The number of Rs1 must be even "


It would be great if the error/warning messages are starting with lower case letter, and without the period at the end.

Thanks for pointing it out. Fixed it the latest commit.

Nelson1225 · 2021-04-26T02:18:54Z

gas/testsuite/gas/riscv/insn-dsp.d

@@ -0,0 +1,245 @@
+#as: -mext-dsp


This looks weird, I don't see any implementation about the option in this PR.

I updated all test cases in the latest commit. Thanks for spotting it out!

Nelson1225 · 2021-04-26T02:28:35Z

include/opcode/riscv.h

@@ -63,6 +63,8 @@ static const char * const riscv_pred_succ[16] =

 #define EXTRACT_ITYPE_IMM(x) \
  (RV_X(x, 20, 12) | (RV_IMM_SIGN(x) << 12))
+#define EXTRACT_ITYPE_IMM6L(x) \


ITYPE or SBTYPE? Is this a typo?

Thanks. Fixed it the latest commit.

Nelson1225 · 2021-04-26T02:39:32Z

gas/testsuite/gas/riscv/insn-dsp64.d

+
+0+000 <dsp64>:
+[ 	]+.*:[ 	]+.*[ 	]+add32[ 	]+r1,r2,r3
+.*R_RISCV_RELAX_ENTRY.*


We don't have this relocation. If you need new relocations, you should file an issue to riscv-psabi first, we need the approvals of riscv community, and developers of gnu and llvm.

Test cases are updated in the latest commit. Thanks for spotting it out!

kito-cheng · 2021-04-26T08:33:50Z

nds means abbreviation of andes, which used in Andestech internal only, P extension will become standard extension rather than Andestech specific extension, so I think nds should gone in this implementation.

linsinan1995 · 2021-04-26T09:05:34Z

Hi @kito-cheng , I did not find a proper prefix for p ext operands, so keep 'nds' as prefix (since old spec also used it as a prefix for intrinsic functions). Prefixes like 'p', 'P', 'r' (rvp) and 'd' (dsp) are taken. I just filed an issue for this problem. Thanks.

Nelson1225 · 2021-04-26T09:36:19Z

I shouldn't have time to check the encoding, does the encoding conflicts are resolved in the v0.93 spec? If they are resolved, and the previous comments are also fixed, then the patches looks good to me. The operand naming doesn't really matter, just renamed to rps or others are fine to me, just avoid using the vendor prefixes since p is a standard extension. Thanks.

linsinan1995 · 2021-04-27T21:51:28Z

I shouldn't have time to check the encoding, does the encoding conflicts are resolved in the v0.93 spec? If they are resolved, and the previous comments are also fixed, then the patches looks good to me. The operand naming doesn't really matter, just renamed to rps or others are fine to me, just avoid using the vendor prefixes since p is a standard extension. Thanks.

I will check those encoding conflicts and then inform you. Thanks.

Nelson1225

Same GNU coding standard Indents, it would great to fix them if you plan to go upstream. Otherwise looks good to me. Thanks.

Nelson1225 · 2021-06-09T04:41:33Z

bfd/elfxx-riscv.c

+  if (riscv_lookup_subset (rps->subset_list, "p", &subset))
+    {
+      riscv_parse_add_subset (rps, "zpn",
+                              RISCV_UNKNOWN_VERSION,


Wrong GNU indents.

Nelson1225 · 2021-06-09T04:43:08Z

gas/config/tc-riscv.c

@@ -131,6 +131,8 @@ static const struct riscv_ext_version ext_version_table[] =
  {"c", ISA_SPEC_CLASS_20190608, 2, 0},
  {"c", ISA_SPEC_CLASS_2P2,      2, 0},

+  {"p", ISA_SPEC_CLASS_DRAFT, 2, 0},


Same as the g and k expansions, but we don't need to care about this here.

Nelson1225 · 2021-06-09T04:45:09Z

gas/config/tc-riscv.c

+    *p++ = *str_t++;
+  *p = '\0';
+
+  if (strncmp (name, "nds_", 4) == 0)


This is minor, so it is OK here. But once it becomes to one of the stable standard extensions, then I think we should give them new operand names.

linsinan1995 · 2021-06-16T18:26:13Z

Some updates on the PR

squash fixup commits
fix wrong indents in bfd/elfxx-riscv.c
change swap16 and smbb32 to be an alias of pkbt16 and mulsr64

Also, I did not find any conflict with the encoding in B ext. Perhaps it has been resolved?

hope51607 · 2021-08-16T15:22:00Z

include/opcode/riscv-opc.h

+#define MASK_SRLI16_U 0xff00707f
+#define MATCH_STAS16 0xf4002077
+#define MASK_STAS16 0xfe00707f
+#define MATCH_STSA16 0x46003077


Hi, according to riscv-p-spec and riscv-opcodes, I think MATCH_STSA16 should be 0xf6002077.

Fixed. thanks!

marcfedorow · 2021-08-03T08:29:29Z

opcodes/riscv-opc.c

@@ -1025,7 +1025,7 @@ const struct riscv_opcode riscv_opcodes[] =
 {"sunpkd831",   0, INSN_CLASS_ZPN, "d,s",       MATCH_SUNPKD831, MASK_SUNPKD831, match_opcode, 0 },
 {"sunpkd832",   0, INSN_CLASS_ZPN, "d,s",       MATCH_SUNPKD832, MASK_SUNPKD832, match_opcode, 0 },
 {"swap8",       0, INSN_CLASS_ZPN, "d,s",       MATCH_SWAP8, MASK_SWAP8, match_opcode, 0 },
-{"swap16",      0, INSN_CLASS_ZPN, "d,s",       MATCH_SWAP16, MASK_SWAP16, match_opcode, 0 },
+{"swap16",      0, INSN_CLASS_ZPN, "d,s,t",       MATCH_PKBT16, MASK_PKBT16, match_rs1_eq_rs2, INSN_ALIAS },


How does this work?
I do not think that it is correct -- and if it is, I do not understand why.
I think if you try to compile some assembler lines with this insn, you get now

Error: illegal operands `swap16 x1, x2'

while this requires three registers and SWAP16 requires just two.

totally agree. thanks a lot for catching up! I have made a fix on this.

gas/testsuite/gas/riscv/insn-dsp.s

marcfedorow · 2021-08-18T13:50:35Z

include/opcode/riscv.h

@@ -229,6 +253,8 @@ static const char * const riscv_pred_succ[16] =
 #define OP_SH_AQ		26
 #define OP_MASK_RL		0x1
 #define OP_SH_RL		25
+#define OP_MASK_RC		0x1f
+#define OP_SH_RC		27


OP_MASK_RS3

marcfedorow · 2021-08-18T13:51:32Z

opcodes/riscv-opc.c

+{"ave",         0, INSN_CLASS_ZPN, "d,s,t",     MATCH_AVE, MASK_AVE, match_opcode, 0 },
+{"bitrev",      0, INSN_CLASS_ZPN, "d,s,t",     MATCH_BITREV, MASK_BITREV, match_opcode, 0 },
+{"bitrevi",     0, INSN_CLASS_ZPN, "d,s,l",     MATCH_BITREVI, MASK_BITREVI, match_opcode, 0 },
+{"bpick",       0, INSN_CLASS_ZPN, "d,s,t,nds_rc", MATCH_BPICK, MASK_BPICK, match_opcode, 0 },


nds_rc => r

It looks to me that nds_rc can be replaced by r with the patch.

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c index e6b5cba205..f20dc3db64 100644 --- a/opcodes/riscv-dis.c +++ b/opcodes/riscv-dis.c @@ -306,6 +306,11 @@ print_insn_args (const char *d, insn_t l, bfd_vma pc, disassemble_info *info) print (info->stream, "%s", riscv_gpr_names[rs1]); break; + case 'r': + print (info->stream, "%s", + riscv_gpr_names[EXTRACT_OPERAND (RS3, l)]); + break; +

The regression test passes, but I am not sure about whether this change would affect the behaviour of .insn directive. Hi @Nelson1225 , any suggestion on it?

linsinan1995 · 2022-02-13T14:12:55Z

the new commit fixes an error that csr* instructions can't access RVP csr, and adds a test case.

Nelson1225 reviewed Apr 26, 2021

View reviewed changes

linsinan1995 mentioned this pull request Apr 26, 2021

Overlappings on the operand name for p ext #258

Open

Nelson1225 approved these changes Jun 9, 2021

View reviewed changes

linsinan1995 force-pushed the riscv-binutils-experiment-p-ext branch from 9e69931 to 7970eb9 Compare June 16, 2021 18:15

linsinan1995 changed the title ~~Add p-ext support with spec v0.93~~ Add p-ext support with spec v0.94 Jun 16, 2021

hope51607 reviewed Aug 16, 2021

View reviewed changes

linsinan1995 and others added 4 commits August 16, 2021 21:41

Add p-ext support with spec v0.94

92f414d

Fix disassmeble kmada32 to it alais name kmar64

45fd911

Sync version flag with GCC

99cb49a

set swap16 and smbb32 alias to pkbt16 and mulsr64

a638e73

linsinan1995 force-pushed the riscv-binutils-experiment-p-ext branch from 7970eb9 to 46a9dbb Compare August 16, 2021 18:44

marcfedorow reviewed Aug 18, 2021

View reviewed changes

linsinan1995 added 7 commits August 18, 2021 22:30

fix wrong format of swap16 & add testcases

e9818b4

update to 0.96: remove zprv

04ede4d

update to 0.96: remove clo*

09ef7ea

remove rv64 add/sub 64

79182e2

update testcase to spec 0.9.10

9377740

update to spec 0.9.10

1b71bd8

disassemble rs3 argument ('r' modifiler)

e78ce2b

linsinan1995 force-pushed the riscv-binutils-experiment-p-ext branch from 2217f9b to e78ce2b Compare November 9, 2021 01:25

linsinan1995 added 2 commits November 9, 2021 19:57

remove minw, maxw and bpick

ebd2150

fix rdov and clrov and remove persudo insn in DECLARE_INSN

dcfc944

linsinan1995 force-pushed the riscv-binutils-experiment-p-ext branch from 87cd9ed to dcfc944 Compare February 13, 2022 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add p-ext support with spec v0.94 #257

Add p-ext support with spec v0.94 #257

linsinan1995 commented Apr 23, 2021

Nelson1225 left a comment •

edited

Loading

Nelson1225 Apr 26, 2021

linsinan1995 Apr 26, 2021

Nelson1225 Apr 26, 2021

linsinan1995 Apr 26, 2021

Nelson1225 Apr 26, 2021

linsinan1995 Apr 26, 2021

Nelson1225 Apr 26, 2021

linsinan1995 Apr 26, 2021

Nelson1225 Apr 26, 2021

linsinan1995 Apr 26, 2021

Nelson1225 Apr 26, 2021

linsinan1995 Apr 26, 2021

Nelson1225 Apr 26, 2021

linsinan1995 Apr 26, 2021

kito-cheng commented Apr 26, 2021

linsinan1995 commented Apr 26, 2021 •

edited

Loading

Nelson1225 commented Apr 26, 2021

linsinan1995 commented Apr 27, 2021

Nelson1225 left a comment •

edited

Loading

Nelson1225 Jun 9, 2021

Nelson1225 Jun 9, 2021

Nelson1225 Jun 9, 2021

linsinan1995 commented Jun 16, 2021 •

edited

Loading

hope51607 Aug 16, 2021

linsinan1995 Aug 16, 2021

marcfedorow Aug 3, 2021 •

edited

Loading

linsinan1995 Aug 18, 2021

marcfedorow Aug 18, 2021

marcfedorow Aug 18, 2021

linsinan1995 Aug 18, 2021

linsinan1995 commented Feb 13, 2022

Add p-ext support with spec v0.94 #257

Are you sure you want to change the base?

Add p-ext support with spec v0.94 #257

Conversation

linsinan1995 commented Apr 23, 2021

Nelson1225 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kito-cheng commented Apr 26, 2021

linsinan1995 commented Apr 26, 2021 • edited Loading

Nelson1225 commented Apr 26, 2021

linsinan1995 commented Apr 27, 2021

Nelson1225 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linsinan1995 commented Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcfedorow Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linsinan1995 commented Feb 13, 2022

Nelson1225 left a comment •

edited

Loading

linsinan1995 commented Apr 26, 2021 •

edited

Loading

Nelson1225 left a comment •

edited

Loading

linsinan1995 commented Jun 16, 2021 •

edited

Loading

marcfedorow Aug 3, 2021 •

edited

Loading