Skip to content
This repository has been archived by the owner on Aug 17, 2022. It is now read-only.

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

Open
wants to merge 13 commits into
base: riscv-binutils-experiment
Choose a base branch
from

Conversation

linsinan1995
Copy link

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

Copy link
Collaborator

@Nelson1225 Nelson1225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#define MAX_KEYWORD_LEN 32

static bfd_boolean
parse_nds_v5_field (const char **str, char name[MAX_KEYWORD_LEN])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‘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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@@ -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 */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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) \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ITYPE or SBTYPE? Is this a typo?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Fixed it the latest commit.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kito-cheng
Copy link
Collaborator

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
Copy link
Author

linsinan1995 commented Apr 26, 2021

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
Copy link
Collaborator

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
Copy link
Author

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.

Copy link
Collaborator

@Nelson1225 Nelson1225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong GNU indents.

@@ -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},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*p++ = *str_t++;
*p = '\0';

if (strncmp (name, "nds_", 4) == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 linsinan1995 force-pushed the riscv-binutils-experiment-p-ext branch from 9e69931 to 7970eb9 Compare June 16, 2021 18:15
@linsinan1995 linsinan1995 changed the title Add p-ext support with spec v0.93 Add p-ext support with spec v0.94 Jun 16, 2021
@linsinan1995
Copy link
Author

linsinan1995 commented Jun 16, 2021

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. thanks!

@linsinan1995 linsinan1995 force-pushed the riscv-binutils-experiment-p-ext branch from 7970eb9 to 46a9dbb Compare August 16, 2021 18:44
@@ -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 },
Copy link

@marcfedorow marcfedorow Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

gas/testsuite/gas/riscv/insn-dsp.s Outdated Show resolved Hide resolved
gas/testsuite/gas/riscv/insn-dsp.s Outdated Show resolved Hide resolved
@@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OP_MASK_RS3

{"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 },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nds_rc => r

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 linsinan1995 force-pushed the riscv-binutils-experiment-p-ext branch from 2217f9b to e78ce2b Compare November 9, 2021 01:25
@linsinan1995
Copy link
Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants