回复：Re: Re: 回复： Re: RISCV: Support Zilsd and Zcmlsd extensions.

孙东亚 sundongya@nucleisys.com
Wed Mar 19 03:28:33 GMT 2025 

Thanks for your valuable suggestions. 

First, regarding the certification matter, I would like to mention that our company had previously completed the signing process. The name of the signature package we received is Nuclei.System.Technology.1694426.GNU.tar.gz. We would appreciate it if you could help us double-check this to ensure everything is in order. 


In addition, the code has been adjusted according to the suggestions to make it more standardized.  The spec link: https://github.com/riscv/riscv-zilsd/blob/main/zilsd.adoc


We sincerely apologize for the delay in our response. Due to an oversight, we only replied to Nelson Chu<nelson@rivosinc.com> on February 6, 2025.


We're more than happy to keep working on the patch to make it even better. Looking forward to your reply.


bfd/ChangeLog:

        * elfxx-riscv.c (riscv_parse_check_conflicts):"zclsd" conflict with the “c+f'/ zcf”extension.
        (riscv_multi_subset_supports):Handle zilsd and zclsd.
        (riscv_multi_subset_supports_ext):Handle zilsd and zclsd.


gas/ChangeLog:


        * testsuite/gas/riscv/zclsd-fail.d: New test.
        * testsuite/gas/riscv/zclsd-fail.l: New test.
        * testsuite/gas/riscv/zclsd-fail.s: New test.
        * testsuite/gas/riscv/zclsd.d: New test.
        * testsuite/gas/riscv/zclsd.s: New test.
        * testsuite/gas/riscv/zilsd-fail.d: New test.
        * testsuite/gas/riscv/zilsd-fail.l: New test.
        * testsuite/gas/riscv/zilsd-fail.s: New test.
        * testsuite/gas/riscv/zilsd.d: New test.
        * testsuite/gas/riscv/zilsd.s: New test.


include/ChangeLog:


        * opcode/riscv.h (enum riscv_insn_class):Add INSN_CLASS_ZILSD and INSN_CLASS_ZCLSD.


opcodes/ChangeLog:


        * riscv-opc.c (MASK_CRS2S):Define the crs2s register operand mask.
        (match_rd_even):Constrain the rd register operand.
        (match_rs2_even):Constrain the rs2 register operand.
        (match_rd_even_nonzero):Constrain the rd register operand.
        (match_rs1_nonzero_rs2_even):Constrain the rs1 and rs2 register operand.
        (match_crs2s_even):Constrain the crs2s register operand.
        (match_crs2_even):Constrain the crs2 register operand.


---
 bfd/elfxx-riscv.c                    | 20 ++++++++++++++++++++ gas/testsuite/gas/riscv/zclsd-fail.d |  3 +++
 gas/testsuite/gas/riscv/zclsd-fail.l | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gas/testsuite/gas/riscv/zclsd-fail.s | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gas/testsuite/gas/riscv/zclsd.d      | 27 +++++++++++++++++++++++++++
 gas/testsuite/gas/riscv/zclsd.s      | 18 ++++++++++++++++++
 gas/testsuite/gas/riscv/zilsd-fail.d |  3 +++
 gas/testsuite/gas/riscv/zilsd-fail.l | 22 ++++++++++++++++++++++
 gas/testsuite/gas/riscv/zilsd-fail.s | 22 ++++++++++++++++++++++
 gas/testsuite/gas/riscv/zilsd.d      | 20 ++++++++++++++++++++
 gas/testsuite/gas/riscv/zilsd.s      | 11 +++++++++++
 include/opcode/riscv.h               |  2 ++

 opcodes/riscv-opc.c                  | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 
 13 files changed, 306 insertions(+)





                    Nelson Chu<nelson@rivosinc.com> 在 2025年1月7日 周二 14:33 写道：

Reviewed and attached patch, looks good, thanks.


Just some minor stuff,


1. Copyright or DCO (Developer's Certificate of Origin)
Btw, this isn't minor though.  Feel free to tell me if you or your company has signed the copyright, since I cannot find either for the NUCLEI system.  Or alternative is to sign off the contribution by agreeing to the DCO.  Please check the details in the binutils/MAINTAINERS file.
https://sourceware.org/pipermail/binutils/2022-October/123680.html


2. It would be great to also add the spec link in the commit comments.  I think it should be,
https://github.com/riscv/riscv-zilsd/blob/main/zilsd.adoc#insns-ld


3. Coding standard issues.  According to the gnu coding standards,
https://www.gnu.org/prep/standards/standards.html


+static int
+match_rd_even_nonzero (const struct riscv_opcode *op, insn_t insn)
+{
+  return match_rd_nonzero(op, insn) && match_rd_even(op, insn);
+}

+ 
+static int
+match_rs1_nonzero_rs2_even (const struct riscv_opcode *op ATTRIBUTE_UNUSED, insn_t insn)
+{
+  return match_rs1_nonzero(op, insn) && match_rs2_even(op, insn);
+}


Need space before the open-parentheses.

+static int
+match_crs2s_even(const struct riscv_opcode *op, insn_t insn)
+{
+  int crs2s = (insn & MASK_CRS2S) >> OP_SH_CRS2S;
+  return ((crs2s & 1)==0) && match_opcode (op, insn);
+}
+
+static int
+match_crs2_even(const struct riscv_opcode *op, insn_t insn)
+{
+  int crs2 = (insn & MASK_CRS2) >> OP_SH_CRS2;
+  return ((crs2 & 1)==0) && match_opcode (op, insn);
+}


Likewise.  Also add spaces before and after operators like "==" here, though the standard page doesn't mention it in detail, but we used to add like that.


@@ -1127,9 +1176,14 @@ const struct riscv_opcode riscv_opcodes[] =
 {"c.addiw",   64, INSN_CLASS_C,   "d,Co",      MATCH_C_ADDIW, MASK_C_ADDIW, match_rd_nonzero, 0 },
 {"c.addw",    64, INSN_CLASS_C,   "Cs,Ct",     MATCH_C_ADDW, MASK_C_ADDW, match_opcode, 0 },
 {"c.subw",    64, INSN_CLASS_C,   "Cs,Ct",     MATCH_C_SUBW, MASK_C_SUBW, match_opcode, 0 },
+/*zilsd and c*/
+{"c.ldsp",    32, INSN_CLASS_ZCLSD, "d,Cn(Cc)",  MATCH_C_LDSP, MASK_C_LDSP, match_rd_even_nonzero, INSN_DREF|INSN_8_BYTE },
 {"c.ldsp",    64, INSN_CLASS_C,   "d,Cn(Cc)",  MATCH_C_LDSP, MASK_C_LDSP, match_rd_nonzero, INSN_DREF|INSN_8_BYTE },


Please put two spaces after the end of a sentence in your comments, so that the Emacs sentence commands will work.
For example, /* Sentence.  */


Thanks
Nelson


On Thu, Dec 19, 2024 at 5:57 PM 孙东亚 <sundongya@nucleisys.com> wrote:

Thanks for your suggestions, and based on your advice, we have made the following adjustments to the code:
1. Fixed the error where the register operands of the Zclsd insn did not meet the even number constraint.
2. Instead of using the new operands Gd and Gt, we have created new functions such as match_rd_even, match_rs2_even, and match_crs2s_even to constrain the register operands, as suggested.
3. We've combined the insns from the same extension into groups to improve the efficiency of testcases .




                    Jan Beulich<jbeulich@suse.com> 在 2024年11月27日 周三 16:06 写道：

On 27.11.2024 08:20, 孙东亚 wrote:
> Thanks for your advise and based on the provided suggestions, the following modifications have been made:
> 1.&nbsp;Renamed zcmlsd to zclsd according to upstream changes : https://github.com/riscv/riscv-zilsd.
> 
> 2.&nbsp;Re-adapted the patch on the master branch based on the latest version of binutils.
> 3.&nbsp;Modified the indentation of the patch to make it more standardized.
> 4.&nbsp;For all new instructions, a test case has been added.

Why is it that the Zilsd insns have their register operands properly constrained
to even numbers, but the Zclsd ones don't? You even appear to have wrong uses of
the insns in the supposedly-valid testcases.

I further wonder why you introduce new Gd and Gt operand forms, when Zacas handles
the quite similar situation for the pair AMOCAS forms differently, without
consuming another precious letter ('G'). Consistency in handling is an imo
worthwhile goal.

As to the splitting of testcases: Each test comes with some overhead. Therefore
I'd like to recommend that testcases for the different insns of the same
extension be combined, resulting in slightly better time and space efficiency.

Also please take care of the various "\ No newline at end of file" in the patch.

Jan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/binutils/attachments/20250319/c8f50b98/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: zilsd-zclsd.patch
Type: application/octet-stream
Size: 17466 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20250319/c8f50b98/attachment-0001.obj>

More information about the Binutils mailing list