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

[Nelson Chu]

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. Renamed zcmlsd to zclsd according to upstream changes :
> https://github.com/riscv/riscv-zilsd.
> >
> > 2. Re-adapted the patch on the master branch based on the latest
> version of binutils.
> > 3. Modified the indentation of the patch to make it more
> standardized.
> > 4. 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/20250107/3d4048e2/attachment.htm>