Re: Re: [PATCH v2] RISC-V: Support Zca/f/d extensions.

[jiawei@iscas.ac.cn]

[-- Attachment #1: Type: text/plain, Size: 15742 bytes --]

>Thanks, just some minor issues as follows, ...
>
>On Tue, Jul 11, 2023 at 4:33 PM Jiawei <jiawei@iscas.ac.cn> wrote:
>
>> This patch add Zca/f/d extensions support, since all ZC*
>> extensions will imply Zca extension, just enabled compress
>> feature when Zca extension is available.
>>
>> V2: Merged patches and refined testcases as Nelson's suggestions.
>>
>> 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.
>>         (riscv_multi_subset_supports_ext): Ditto.
>>
>> gas/ChangeLog:
>>
>>         * config/tc-riscv.c (riscv_set_arch): Extend compress check.
>>         * testsuite/gas/riscv/zca.d: New test.
>>         * testsuite/gas/riscv/zca.s: New test.
>>         * testsuite/gas/riscv/zcd.d: New test.
>>         * testsuite/gas/riscv/zcd.s: New test.
>>         * testsuite/gas/riscv/zcf.d: New test.
>>         * testsuite/gas/riscv/zcf.s: New test.
>>
>> ---
>>  bfd/elfxx-riscv.c             | 28 ++++++++++++------
>>  gas/config/tc-riscv.c         |  3 +-
>>  gas/testsuite/gas/riscv/zca.d | 54 +++++++++++++++++++++++++++++++++++
>>  gas/testsuite/gas/riscv/zca.s | 47 ++++++++++++++++++++++++++++++
>>  gas/testsuite/gas/riscv/zcd.d | 16 +++++++++++
>>  gas/testsuite/gas/riscv/zcd.s | 10 +++++++
>>  gas/testsuite/gas/riscv/zcf.d | 16 +++++++++++
>>  gas/testsuite/gas/riscv/zcf.s | 10 +++++++
>>  8 files changed, 174 insertions(+), 10 deletions(-)
>>  create mode 100644 gas/testsuite/gas/riscv/zca.d
>>  create mode 100644 gas/testsuite/gas/riscv/zca.s
>>  create mode 100644 gas/testsuite/gas/riscv/zcd.d
>>  create mode 100644 gas/testsuite/gas/riscv/zcd.s
>>  create mode 100644 gas/testsuite/gas/riscv/zcf.d
>>  create mode 100644 gas/testsuite/gas/riscv/zcf.s
>>
>> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
>> index bdfc0ef01f4..25864a8cf89 100644
>> --- a/bfd/elfxx-riscv.c
>> +++ b/bfd/elfxx-riscv.c
>> @@ -1171,6 +1171,8 @@ static struct riscv_implicit_subset
>> riscv_implicit_subsets[] =
>>    {"zvksg", "zvkg",    check_implicit_always},
>>    {"zvksc", "zvks",    check_implicit_always},
>>    {"zvksc", "zvbc",    check_implicit_always},
>> +  {"zcf", "zca",       check_implicit_always},
>> +  {"zcd", "zca",       check_implicit_always},
>
>
>1. Should we also imply f for zcf and d for zcd?

I think it's really a good idea to add this imply relationship.
It's very convenient for users,I will add it in next version.

>2. Is c totally the same as zca for now?  Or c includes all rvc
>instructions, like zca, zcf, zcd and zcb, ...  It seems the latter makes
>more sense.

You are right，it means the latter one. However, all zc* extension will 
defaultly implying zca. It changes the relationship, zca is the public part
when use other zc* extensions.

Here is an example, when we want to zcb extension, it should generate compress
code and set the align with 16 bits, what we really use is (zca + zcb), we can
choice zca or zcb to enable the compress feature as c extension did. If case change
to zcmp(zcmp = zca + zcmp), we choice form zca or zcmp again. So I just set zca 
to do this work as c extension did.

>
>
>>    {"smaia", "ssaia",           check_implicit_always},
>>    {"smstateen", "ssstateen",   check_implicit_always},
>>    {"smepmp", "zicsr",          check_implicit_always},
>> @@ -1304,6 +1306,9 @@ 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 },
>> +  {"zcf",              ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
>> +  {"zcd",              ISA_SPEC_CLASS_DRAFT,           1, 0,  0 },
>>    {NULL, 0, 0, 0, 0}
>>  };
>>
>> @@ -2384,13 +2389,16 @@ riscv_multi_subset_supports (riscv_parse_subset_t
>> *rps,
>>      case INSN_CLASS_Q:
>>        return riscv_subset_supports (rps, "q");
>>      case INSN_CLASS_C:
>>
>
>Renamed to INSN_CLASS_ZCA?

I'm a little concerned if this change will cause any compatibility issues, if not,
it's a good choice.

>
>
>> -      return riscv_subset_supports (rps, "c");
>> +      return riscv_subset_supports (rps, "c")
>> +             || riscv_subset_supports (rps, "zca");
>>      case INSN_CLASS_F_AND_C:
>>
>
>Renamed to INSN_CLASS_F_AND_ZCF or INSN_CLASS_ZCF when implying f?
>
>
>>        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:
>>
>
>Renamed to INSN_CLASS_F_AND_ZCD or INSN_CLASS_ZCD when implying d?
>
>
>>        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"));
>> @@ -2566,20 +2574,22 @@ riscv_multi_subset_supports_ext
>> (riscv_parse_subset_t *rps,
>>        return "c";
>>      case INSN_CLASS_F_AND_C:
>>        if (!riscv_subset_supports (rps, "f")
>> -         && !riscv_subset_supports (rps, "c"))
>> -       return _("f' and `c");
>> +         && !(riscv_subset_supports (rps, "c")
>> +               || riscv_subset_supports (rps, "zcf")))
>> +       return _("f' and `c', or `f' and `zcf");
>>        else if (!riscv_subset_supports (rps, "f"))
>>         return "f";
>>        else
>> -       return "c";
>> +       return _("c' or `zcf");
>
>
>case INSN_CLASS_F_AND_ZCF:
>  if (!riscv_subset_supports (rps, "f")
>    {
>      if (!riscv_subset_supports (rps, "c") && !riscv_subset_supports (rps,
>"zcf"))
>        return _("f' and `c', or `f' and `zcf");
>      else
>        return "f";
>    }
>  else
>    return _("c' or `zcf");
>
>Or if we imply f for zcf, then just,
>
>case INSN_CLASS_ZCF:
>  return _("c' or `zcf");
>

Thanks for your demo.

>
>>      case INSN_CLASS_D_AND_C:
>>        if (!riscv_subset_supports (rps, "d")
>> -         && !riscv_subset_supports (rps, "c"))
>> -       return _("d' and `c");
>> +         && !(riscv_subset_supports (rps, "c")
>> +               || riscv_subset_supports (rps, "zcd")))
>> +       return _("d' and `c', or `d' and `zcd");
>>        else if (!riscv_subset_supports (rps, "d"))
>>         return "d";
>>        else
>> -       return "c";
>> +       return _("c' or `zcd");
>>
>
>Likewise.
>
>
>>      case INSN_CLASS_F_INX:
>>        return _("f' or `zfinx");
>>      case INSN_CLASS_D_INX:
>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> index 297bb9b2a81..2eb0d1877ee 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 c is just the same as zca, then it looks good.  But if c includes not
>only zca, then this needs to be changed.
>
>
>>    if (riscv_subset_supports (&riscv_rps_as, "ztso"))
>> diff --git a/gas/testsuite/gas/riscv/zca.d b/gas/testsuite/gas/riscv/zca.d
>> new file mode 100644
>> index 00000000000..86493985e70
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/zca.d
>> @@ -0,0 +1,54 @@
>> +#as: -march=rv64i_zca
>> +#objdump: -d -Mno-aliases
>> +
>> +.*:[   ]+file format .*
>> +
>> +Disassembly of section .text:
>> +
>> +0+000 <target>:
>> +[      ]+[0-9a-f]+:[   ]+40fd[         ]+c.li[         ]+ra,31
>> +[      ]+[0-9a-f]+:[   ]+4101[         ]+c.li[         ]+sp,0
>> +[      ]+[0-9a-f]+:[   ]+6085[         ]+c.lui[        ]+ra,0x1
>> +[      ]+[0-9a-f]+:[   ]+61fd[         ]+c.lui[        ]+gp,0x1f
>> +[      ]+[0-9a-f]+:[   ]+4080[         ]+c.lw[         ]+s0,0\(s1\)
>> +[      ]+[0-9a-f]+:[   ]+5104[         ]+c.lw[         ]+s1,32\(a0\)
>> +[      ]+[0-9a-f]+:[   ]+4502[         ]+c.lwsp[       ]+a0,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+4082[         ]+c.lwsp[       ]+ra,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+6380[         ]+c.ld[         ]+s0,0\(a5\)
>> +[      ]+[0-9a-f]+:[   ]+6504[         ]+c.ld[         ]+s1,8\(a0\)
>> +[      ]+[0-9a-f]+:[   ]+6502[         ]+c.ldsp[       ]+a0,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+6082[         ]+c.ldsp[       ]+ra,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+c080[         ]+c.sw[         ]+s0,0\(s1\)
>> +[      ]+[0-9a-f]+:[   ]+d104[         ]+c.sw[         ]+s1,32\(a0\)
>> +[      ]+[0-9a-f]+:[   ]+c02a[         ]+c.swsp[       ]+a0,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+c006[         ]+c.swsp[       ]+ra,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+e380[         ]+c.sd[         ]+s0,0\(a5\)
>> +[      ]+[0-9a-f]+:[   ]+e504[         ]+c.sd[         ]+s1,8\(a0\)
>> +[      ]+[0-9a-f]+:[   ]+e02a[         ]+c.sdsp[       ]+a0,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+e006[         ]+c.sdsp[       ]+ra,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+0001[         ]+c.addi[       ]+zero,0
>> +[      ]+[0-9a-f]+:[   ]+0001[         ]+c.addi[       ]+zero,0
>> +[      ]+[0-9a-f]+:[   ]+007d[         ]+c.addi[       ]+zero,31
>> +[      ]+[0-9a-f]+:[   ]+908a[         ]+c.add[        ]+ra,sp
>> +[      ]+[0-9a-f]+:[   ]+05fd[         ]+c.addi[       ]+a1,31
>> +[      ]+[0-9a-f]+:[   ]+0101[         ]+c.addi[       ]+sp,0
>> +[      ]+[0-9a-f]+:[   ]+25fd[         ]+c.addiw[      ]+a1,31
>> +[      ]+[0-9a-f]+:[   ]+2101[         ]+c.addiw[      ]+sp,0
>> +[      ]+[0-9a-f]+:[   ]+0040[         ]+c.addi4spn[   ]+s0,sp,4
>> +[      ]+[0-9a-f]+:[   ]+6105[         ]+c.addi16sp[   ]+sp,32
>> +[      ]+[0-9a-f]+:[   ]+9c25[         ]+c.addw[       ]+s0,s1
>> +[      ]+[0-9a-f]+:[   ]+8c05[         ]+c.sub[        ]+s0,s1
>> +[      ]+[0-9a-f]+:[   ]+9c05[         ]+c.subw[       ]+s0,s1
>> +[      ]+[0-9a-f]+:[   ]+8c65[         ]+c.and[        ]+s0,s1
>> +[      ]+[0-9a-f]+:[   ]+887d[         ]+c.andi[       ]+s0,31
>> +[      ]+[0-9a-f]+:[   ]+8c45[         ]+c.or[         ]+s0,s1
>> +[      ]+[0-9a-f]+:[   ]+8c25[         ]+c.xor[        ]+s0,s1
>> +[      ]+[0-9a-f]+:[   ]+8006[         ]+c.mv[         ]+zero,ra
>> +[      ]+[0-9a-f]+:[   ]+0006[         ]+c.slli[       ]+zero,0x1
>> +[      ]+[0-9a-f]+:[   ]+0002[         ]+c.slli64[     ]+zero
>> +[      ]+[0-9a-f]+:[   ]+d845[         ]+c.beqz[       ]+s0,0[
>> ]+\<target\>
>> +[      ]+[0-9a-f]+:[   ]+f45d[         ]+c.bnez[       ]+s0,0[
>> ]+\<target\>
>> +[      ]+[0-9a-f]+:[   ]+b775[         ]+c.j[  ]+0[ ]+\<target\>
>> +[      ]+[0-9a-f]+:[   ]+8082[         ]+c.jr[         ]+ra
>> +[      ]+[0-9a-f]+:[   ]+9082[         ]+c.jalr[       ]+ra
>> +[      ]+[0-9a-f]+:[   ]+9002[         ]+c.ebreak
>> diff --git a/gas/testsuite/gas/riscv/zca.s b/gas/testsuite/gas/riscv/zca.s
>> new file mode 100644
>> index 00000000000..1ad4f09eefd
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/zca.s
>> @@ -0,0 +1,47 @@
>> +target:
>> +       c.li x1, 31
>> +       c.li x2, 0
>> +       c.lui x1, 1
>> +       c.lui x3, 31
>> +       c.lw x8, (x9)
>> +       c.lw x9, 32(x10)
>> +       lw a0, (sp)
>> +       c.lwsp x1, (x2)
>> +       c.ld x8, (x15)
>> +       c.ld x9, 8(x10)
>> +       ld a0,(sp)
>> +       c.ldsp x1, (sp)
>> +       c.sw x8, (x9)
>> +       c.sw x9, 32(x10)
>> +       sw a0, (sp)
>> +       c.swsp x1, (x2)
>> +       c.sd x8, (x15)
>> +       c.sd x9, 8(x10)
>> +       sd a0, (sp)
>> +       c.sdsp x1, (sp)
>> +       addi x0, x0, 0
>> +       c.nop
>> +       c.nop 31
>> +       c.add x1, x2
>> +       c.addi a1, 31
>> +       c.addi x2, 0
>> +       c.addiw a1, 31
>> +       c.addiw x2, 0
>> +       c.addi4spn x8, x2, 4
>> +       c.addi16sp x2, 32
>> +       c.addw x8, x9
>> +       c.sub x8, x9
>> +       c.subw x8, x9
>> +       c.and x8, x9
>> +       c.andi x8, 31
>> +       c.or x8, x9
>> +       c.xor x8, x9
>> +       c.mv x0, x1
>> +       c.slli x0, 1
>> +       c.slli64 x0
>> +       c.beqz x8, target
>> +       c.bnez x8, target
>> +       c.j target
>> +       c.jr ra
>> +       c.jalr ra
>> +       c.ebreak
>> diff --git a/gas/testsuite/gas/riscv/zcd.d b/gas/testsuite/gas/riscv/zcd.d
>> new file mode 100644
>> index 00000000000..11723c4ee14
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/zcd.d
>> @@ -0,0 +1,16 @@
>> +#as: -march=rv64id_zcd
>> +#objdump: -d -Mno-aliases
>> +
>> +.*:[   ]+file format .*
>> +
>> +Disassembly of section .text:
>> +
>> +0+000 <target>:
>> +[      ]+[0-9a-f]+:[   ]+2108[         ]+c.fld[        ]+fa0,0\(a0\)
>> +[      ]+[0-9a-f]+:[   ]+200c[         ]+c.fld[        ]+fa1,0\(s0\)
>> +[      ]+[0-9a-f]+:[   ]+2502[         ]+c.fldsp[      ]+fa0,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+2582[         ]+c.fldsp[      ]+fa1,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+a108[         ]+c.fsd[        ]+fa0,0\(a0\)
>> +[      ]+[0-9a-f]+:[   ]+a00c[         ]+c.fsd[        ]+fa1,0\(s0\)
>> +[      ]+[0-9a-f]+:[   ]+a02a[         ]+c.fsdsp[      ]+fa0,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+a02e[         ]+c.fsdsp[      ]+fa1,0\(sp\)
>> diff --git a/gas/testsuite/gas/riscv/zcd.s b/gas/testsuite/gas/riscv/zcd.s
>> new file mode 100644
>> index 00000000000..3c29d323c7b
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/zcd.s
>> @@ -0,0 +1,10 @@
>> +target:
>> +       # ZCD only compress double float instructions
>> +       fld fa0, 0(a0)
>> +       c.fld fa1, 0(s0)
>> +       fld fa0, 0(sp)
>> +       c.fldsp fa1, 0(sp)
>> +       fsd fa0, 0(a0)
>> +       c.fsd fa1, 0(s0)
>> +       fsd fa0, 0(sp)
>> +       c.fsdsp fa1, 0(sp)
>> diff --git a/gas/testsuite/gas/riscv/zcf.d b/gas/testsuite/gas/riscv/zcf.d
>> new file mode 100644
>> index 00000000000..44dee021233
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/zcf.d
>> @@ -0,0 +1,16 @@
>> +#as: -march=rv32if_zcf
>> +#objdump: -d -Mno-aliases
>> +
>> +.*:[   ]+file format .*
>> +
>> +Disassembly of section .text:
>> +
>> +0+000 <target>:
>> +[      ]+[0-9a-f]+:[   ]+6108[         ]+c.flw[        ]+fa0,0\(a0\)
>> +[      ]+[0-9a-f]+:[   ]+600c[         ]+c.flw[        ]+fa1,0\(s0\)
>> +[      ]+[0-9a-f]+:[   ]+6502[         ]+c.flwsp[      ]+fa0,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+6582[         ]+c.flwsp[      ]+fa1,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+e108[         ]+c.fsw[        ]+fa0,0\(a0\)
>> +[      ]+[0-9a-f]+:[   ]+e00c[         ]+c.fsw[        ]+fa1,0\(s0\)
>> +[      ]+[0-9a-f]+:[   ]+e02a[         ]+c.fswsp[      ]+fa0,0\(sp\)
>> +[      ]+[0-9a-f]+:[   ]+e02e[         ]+c.fswsp[      ]+fa1,0\(sp\)
>> diff --git a/gas/testsuite/gas/riscv/zcf.s b/gas/testsuite/gas/riscv/zcf.s
>> new file mode 100644
>> index 00000000000..c98fedb3d6c
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/zcf.s
>> @@ -0,0 +1,10 @@
>> +target:
>> +       # ZCF only compress single float instructions with RV32
>> +       flw fa0, 0(a0)
>> +       c.flw fa1, 0(s0)
>> +       flw fa0, 0(sp)
>> +       c.flwsp fa1, 0(sp)
>> +       fsw fa0, 0(a0)
>> +       c.fsw fa1, 0(s0)
>> +       fsw fa0, 0(sp)
>> +       c.fswsp fa1, 0(sp)
>> --
>> 2.25.1
>>
>>
>Thanks! The test cases look pretty good for now :)
>
>Nelson