[PATCH 1/4] RISC-V: Minimal support of ZC extensions.

[Nelson Chu]

Generally, minimal support is useless.  It would be better to be merged
until we have the full support for the extensions.  According to the
currently sent patches, you divide the current c extension into three sub
extensions - zca, zcf, zcd, and also add test cases for them, and then add
new zcb support.  So ideally, there is no need to have the minimal support
here, a better way is to add the architecture strings with the full support
into the same patch.  As for the zce, zcmt, zcmp, you can let the parser
recognize them until you send the latter patches which truly support them.

Btw, separating the test cases into separate patches is also kind of
useless.  Generally, we should add the test cases to make sure the new
feature or the fix is correct in the same patch.  If you see that - there
were some patches that only added the test cases, that means we forgot to
add them when we supported before.  So it would be good if you can,

1. Has one patch to separate the current z extension into zca, zcf, zcd,
and then add the missing test cases for them, and also make sure there
won't be any overlapped tests with
the current ones.  If you decide to use three separate patches, that's also
fine, one for zca with the test cases and arch support, one for the zcf,
and the remaining for the zcd.

2. Try to merge the whole zca/zcf/zcd test cases into one, or at most two
(for rv32 and rv64), it looks messy and hard to maintain if there are lots
of test cases separately.  That means zca-rv32, zca-rv64, zcf-r32,
zcf-rv64, ....  Or if you can just use c-rv32, c-rv64, then that would be
the perfect situation.  If my understanding is correct, Jan is right.  if
we accept to add the test cases for each instruction, then there is another
problem of consistency - we will need hundreds, or more exaggerated
thousands of test cases, depending on the number of instructions we
supported.  Maksray also discussed this with me a long time ago, he
suggested we should merge the test cases as possible as we can.  And now I
figured that what Jan pointed out is a worthy problem - lots of small tests
generally cost more time and space.  So if we can speed up the test times,
and easily maintain the tests, why not?

3. For the zcb operand, it may be good to follows this commit,
https://github.com/bminor/binutils-gdb/commit/54bca63b5c3714e1032bf32754dbadaff424221a.
So the operand you added, CZb and CZh, can be changed to something like,
Wcb and Wch, which looks more consistent.  Btw, adding the arch string
support of zcb into the same patch here would also look better.


Thanks
Nelson


On Tue, Jun 13, 2023 at 9:24 PM Jiawei <jiawei@iscas.ac.cn> wrote:

> This patch add all ZC* extension base support, and enable compress
> feature when Zca extension enabled.
>
> Co-Authored by: Charlie Keaney <charlie.keaney@embecosm.com>
> Co-Authored by: Mary Bennett <mary.bennett@embecosm.com>
> Co-Authored by: Nandni Jamnadas <nandni.jamnadas@embecosm.com>
> Co-Authored by: Sinan Lin <sinan.lin@linux.alibaba.com>
> Co-Authored by: Simon Cook <simon.cook@embecosm.com>
> Co-Authored by: Shihua Liao <shihua@iscas.ac.cn>
> Co-Authored by: Yulong Shi <yulong@iscas.ac.cn>
>
> bfd/ChangeLog:
>
>         * elfxx-riscv.c (riscv_multi_subset_supports): New extensions.
>
> gas/ChangeLog:
>
>         * config/tc-riscv.c (riscv_set_arch): Extend compress check.
>
> ---
>  bfd/elfxx-riscv.c     | 26 +++++++++++++++++++++++---
>  gas/config/tc-riscv.c |  3 ++-
>  2 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 7f453246449..4a7407b8a34 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -1155,6 +1155,16 @@ static struct riscv_implicit_subset
> riscv_implicit_subsets[] =
>    {"zks", "zbkx",      check_implicit_always},
>    {"zks", "zksed",     check_implicit_always},
>    {"zks", "zksh",      check_implicit_always},
> +  {"zce", "zca",       check_implicit_always},
> +  {"zce", "zcb",       check_implicit_always},
> +  {"zce", "zcf",        check_implicit_always},
> +  {"zce", "zcmp",      check_implicit_always},
> +  {"zce", "zcmt",      check_implicit_always},
> +  {"zcf", "zca",       check_implicit_always},
> +  {"zcd", "zca",       check_implicit_always},
> +  {"zcb", "zca",       check_implicit_always},
> +  {"zcmp", "zca",      check_implicit_always},
> +  {"zcmt", "zca",      check_implicit_always},
>    {"smaia", "ssaia",           check_implicit_always},
>    {"smstateen", "ssstateen",   check_implicit_always},
>    {"smepmp", "zicsr",          check_implicit_always},
> @@ -1272,6 +1282,13 @@ static struct riscv_supported_ext
> riscv_supported_std_z_ext[] =
>    {"zvl32768b",                ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
>    {"zvl65536b",                ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
>    {"ztso",             ISA_SPEC_CLASS_DRAFT,           0, 1,  0 },
> +  {"zca",              ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
> +  {"zcb",              ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
> +  {"zce",              ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
> +  {"zcf",              ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
> +  {"zcd",              ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
> +  {"zcmp",             ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
> +  {"zcmt",             ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
>    {NULL, 0, 0, 0, 0}
>  };
>
> @@ -2335,13 +2352,16 @@ riscv_multi_subset_supports (riscv_parse_subset_t
> *rps,
>      case INSN_CLASS_Q:
>        return riscv_subset_supports (rps, "q");
>      case INSN_CLASS_C:
> -      return riscv_subset_supports (rps, "c");
> +      return riscv_subset_supports (rps, "c")
> +               || riscv_subset_supports (rps, "zca");
>      case INSN_CLASS_F_AND_C:
>        return (riscv_subset_supports (rps, "f")
> -             && riscv_subset_supports (rps, "c"));
> +             && (riscv_subset_supports (rps, "c")
> +                 || riscv_subset_supports (rps, "zcf")));
>      case INSN_CLASS_D_AND_C:
>        return (riscv_subset_supports (rps, "d")
> -             && riscv_subset_supports (rps, "c"));
> +             && (riscv_subset_supports (rps, "c")
> +                 || riscv_subset_supports (rps, "zcd")));
>      case INSN_CLASS_F_INX:
>        return (riscv_subset_supports (rps, "f")
>               || riscv_subset_supports (rps, "zfinx"));
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 7684fa7e06d..1d3860b332f 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -337,7 +337,8 @@ riscv_set_arch (const char *s)
>    riscv_reset_subsets_list_arch_str ();
>
>    riscv_set_rvc (false);
> -  if (riscv_subset_supports (&riscv_rps_as, "c"))
> +  if (riscv_subset_supports (&riscv_rps_as, "c")
> +      || riscv_subset_supports (&riscv_rps_as, "zca"))
>      riscv_set_rvc (true);
>
>    if (riscv_subset_supports (&riscv_rps_as, "ztso"))
> --
> 2.25.1
>
>