Add Zclsd extension YAML #577

Shehrozkashif · 2025-03-28T21:33:18Z

This PR adds the Zclsd extension to the database. Zclsd introduces compressed load/store pair instructions for RV32 by reusing RV64-only instruction encodings, which helps improve code density and performance when leveraging a wider than XLEN memory interface. It defines 16-bit instruction encodings and utilizes the same even-odd register pairing as the Zdinx extension. Additionally, Zclsd depends on the Zilsd and Zca extensions, and it is incompatible with Zcf due to overlapping encodings
Closes #572

arch/ext/Zclsd.yaml

arch/inst/Zclsd/c.ld.yaml

dhower-qc

Comments apply to all the instructions.

arch/inst/Zclsd/c.ld.yaml

arch/ext/Zclsd.yaml

arch/inst/Zclsd/c.ld.yaml

arch/inst/Zclsd/c.ldsp.yaml

arch/inst/Zclsd/c.sdsp.yaml

arch/inst/C/c.ld.yaml

arch/ext/Zclsd.yaml

arch/inst/C/c.ld.yaml

arch/inst/Zclsd/c.ldsp.yaml

arch/inst/Zclsd/c.sd.yaml

arch/inst/Zclsd/c.sdsp.yaml

arch/inst/C/c.sd.yaml

ThinkOpenly · 2025-04-22T15:34:42Z

@Shehrozkashif, did you see that there are "conflicts that must be resolved"?

ThinkOpenly · 2025-04-22T15:40:07Z

arch/inst/Zclsd/c.sd.yaml

When this content has all been merged into arch/inst/C/c.sd.yaml, this file should be removed.

arch/inst/C/c.sd.yaml

arch/inst/C/c.ld.yaml

Shehrozkashif · 2025-04-23T11:23:54Z

@ThinkOpenly yes I can see the conflict Should I keep the new logic, the main branch version, or try to merge both?

AFOliveira · 2025-04-23T18:50:38Z

@ThinkOpenly yes I can see the conflict Should I keep the new logic, the main branch version, or try to merge both?

I know your comment is intended to Paul, but I would say to try to manually merge and see what has been done.

If it is the work you have been doing, probably just review the upstream changes, and if you agree, keep them, if you dont agree just push changes to them

Shehrozkashif · 2025-04-24T14:14:33Z

Thanks for the advice, @AFOliveira! I'll go ahead and manually merge the changes and review the differences. If the upstream changes align with my work, I’ll keep them; if not, I’ll make the necessary adjustments. Appreciate the help!

AFOliveira · 2025-04-24T14:16:37Z

Thanks for the advice, @AFOliveira! I'll go ahead and manually merge the changes and review the differences. If the upstream changes align with my work, I’ll keep them; if not, I’ll make the necessary adjustments. Appreciate the help!

If in doubt, use git blame to understand who did the changes and try reaching out to them, or even ask for the review after you merge

AFOliveira

@Shehrozkashif Can you please resolve conversations that are marked as outdated? Github won't let you merge whilst they aren't marked as resolved and when opening a PR it makes it cluttered

arch/ext/Zclsd.yaml

ayosher · 2025-05-05T14:33:08Z

Folks,
What is the plan? When this extension is planned to proceed?

Shehrozkashif · 2025-05-08T20:04:25Z

@ayosher Apologies for the delay I’ll try to complete it as soon as possible.

arch/inst/C/c.ld.yaml

arch/inst/C/c.sd.yaml

dhower-qc

generally, let's move all the register names to x* (xd, xs1, xs2).

arch/inst/C/c.ld.yaml

dhower-qc · 2025-05-08T20:32:21Z

arch/inst/C/c.ld.yaml

 definedBy:
  anyOf:
    - C
    - Zca
-base: 64
+    - Zclsd
 assembly: xd, imm(xs1)
 encoding:
  match: 011-----------00


We need separate encodings for RV32/RV64 since in RV32 xd cannot be odd.

do I have to put them in seprate files ?

See arch/inst/I/slli.yaml for an example of how to specify multiple encodings in one file.

dhower-qc · 2025-05-08T20:33:59Z

arch/inst/C/c.sd.yaml

@@ -13,6 +13,7 @@ definedBy:
  anyOf:
    - C
    - Zca
+    - Zclsd
 base: 64
 assembly: xs2, imm(xs1)
 encoding:


separate RV32/RV64

arch/inst/C/c.sd.yaml

dhower-qc · 2025-05-08T20:38:42Z

arch/inst/Zclsd/c.ldsp.yaml

+  rd, offset(sp). C.LDSP is only valid when rd≠x0; the code points with rd=x0 are reserved.
+definedBy: Zclsd
+assembly: rd, offset(sp)
+encoding:


separate encoding. RV32 xd == 0 or odd is reserved

arch/inst/Zclsd/c.ldsp.yaml

dhower-qc · 2025-05-08T20:40:12Z

arch/inst/Zclsd/c.sd.yaml

+  offset(rs1').
+definedBy: Zclsd
+assembly: c.sd rs2, offset(rs1)
+encoding:


separate encoding

arch/inst/Zclsd/c.sd.yaml

dhower-qc · 2025-05-08T20:41:04Z

arch/inst/Zclsd/c.sdsp.yaml

+  `sd rs2, offset(sp)`.
+definedBy: Zclsd
+assembly: rs2, offset(sp)
+encoding:


separate encoding

arch/inst/Zclsd/c.ldsp.yaml

arch/inst/Zclsd/c.sd.yaml

arch/inst/Zclsd/c.sdsp.yaml

arch/inst/C/c.ld.yaml

arch/inst/C/c.sd.yaml

arch/inst/Zclsd/c.ldsp.yaml

arch/inst/Zclsd/c.sd.yaml

arch/inst/Zclsd/c.sdsp.yaml

Signed-off-by: Shehroz Kashif <[email protected]>

arch/inst/C/c.ld.yaml

ThinkOpenly · 2025-05-14T13:49:08Z

arch/inst/C/c.ld.yaml

-# SPDX-License-Identifier: BSD-2-Clause
+  if (xlen() == 32) {
+      if (implemented?(ExtensionName::Zclsd)) {
+          Bits<128> val = read_memory<128>(X[creg2reg(xs1)] + imm, $encoding);


The "128" here should be "64". We're loading a register pair. In RV32 mode, each register is 32 bits, so a pair would be 64 bits.

ThinkOpenly · 2025-05-14T13:49:57Z

arch/inst/C/c.ld.yaml

+          X[creg2reg(xd)] = val[63:0];
+          X[creg2reg(xd + 1)] = val[127:64];


And these ranges should, respectively, be: 31:0 and 63:32.

ThinkOpenly · 2025-05-14T13:57:43Z

arch/inst/C/c.ld.yaml

              _ => report_invalid_width(__FILE__, __LINE__, width, "load")
            }
        }
    }
  }
-


This needs to be retained, at the end of the Sail code.

ThinkOpenly · 2025-05-14T14:02:15Z

arch/inst/Zclsd/c.ldsp.yaml

+      - name: xd
+        location: 11-7


add a "not" statement here, also including "0" as an invalid value.

Oh, wait. This instruction also exists at arch/inst/c.ldsp.yaml. You need to do the same sort of merging with this instruction into that file as you did from c.ld.yaml. Does that make sense?

This is also true for c.sdsp.

yes ill add it in both

ThinkOpenly · 2025-05-14T14:06:39Z

arch/inst/Zclsd/c.sd.yaml

remove this file

can you please explain the reason of removing it ?

Because you're now defining the RV32 version of this instruction in arch/inst/C/c.sd.yaml, so this file is redundant.

ThinkOpenly · 2025-05-14T14:07:54Z

arch/inst/C/c.sd.yaml

 definedBy:
  anyOf:
    - C
    - Zca
+    - Zclsd
 base: 64


remove this line, since it can be defined for RV32 with your changes.

ThinkOpenly · 2025-05-14T14:08:56Z

arch/inst/Zclsd/c.ldsp.yaml

+  if (xd == 0) {
+    raise(ExceptionCode::IllegalInstruction, mode(), $encoding);
+  }
+
+  if (xlen() == 32 && (creg2reg(xd) % 2 != 0)) {
+    raise(ExceptionCode::IllegalInstruction, mode(), $encoding);
+  }


with the "not" statement in place, you don't need either of these two checks.

ThinkOpenly · 2025-05-14T14:09:53Z

arch/inst/Zclsd/c.sdsp.yaml

The RV32 changes here need to be merged into arch/inst/c.sdsp.yaml, then this file removed.

Shehrozkashif requested a review from dhower-qc as a code owner March 28, 2025 21:33

ThinkOpenly reviewed Mar 30, 2025

View reviewed changes

arch/ext/Zclsd.yaml Outdated Show resolved Hide resolved

ThinkOpenly reviewed Mar 31, 2025

View reviewed changes

arch/ext/Zclsd.yaml Outdated Show resolved Hide resolved

Shehrozkashif force-pushed the Zclsd_extension branch from d37b1aa to e322a4d Compare March 31, 2025 14:04

Add Zclsd extension YAML

abc4553

Shehrozkashif force-pushed the Zclsd_extension branch from e322a4d to abc4553 Compare March 31, 2025 16:17

Shehrozkashif force-pushed the Zclsd_extension branch 2 times, most recently from 02d3cc1 to fb4e75d Compare April 7, 2025 19:40

Shehrozkashif requested a review from AFOliveira April 7, 2025 20:32

jordancarlin reviewed Apr 8, 2025

View reviewed changes

arch/inst/Zclsd/c.ld.yaml Outdated Show resolved Hide resolved

Shehrozkashif force-pushed the Zclsd_extension branch from fb4e75d to 7a01aee Compare April 8, 2025 14:49

dhower-qc requested changes Apr 8, 2025

View reviewed changes

Shehrozkashif force-pushed the Zclsd_extension branch from 7a01aee to ab9a65c Compare April 10, 2025 15:33

Shehrozkashif requested a review from ThinkOpenly April 11, 2025 14:06

ThinkOpenly requested changes Apr 11, 2025

View reviewed changes

Shehrozkashif force-pushed the Zclsd_extension branch from 875af8d to 3e10a3f Compare April 11, 2025 22:23

ThinkOpenly mentioned this pull request Apr 14, 2025

Add conflicts: Zclsd entry in Zcf.yaml for completeness #610

Open

Shehrozkashif force-pushed the Zclsd_extension branch from 3e10a3f to d5263f9 Compare April 16, 2025 21:40

Shehrozkashif requested a review from ThinkOpenly April 16, 2025 21:56

ThinkOpenly requested changes Apr 17, 2025

View reviewed changes

Shehrozkashif force-pushed the Zclsd_extension branch from d5263f9 to addf131 Compare April 18, 2025 22:05

ThinkOpenly reviewed Apr 19, 2025

View reviewed changes

arch/inst/C/c.sd.yaml Outdated Show resolved Hide resolved

Shehrozkashif force-pushed the Zclsd_extension branch from addf131 to 804d7cb Compare April 22, 2025 10:37

Shehrozkashif requested a review from ThinkOpenly April 22, 2025 11:15

ThinkOpenly requested changes Apr 22, 2025

View reviewed changes

add Zclsd instructions

d49cdeb

Shehrozkashif force-pushed the Zclsd_extension branch from 804d7cb to d49cdeb Compare April 24, 2025 21:11

AFOliveira reviewed Apr 29, 2025

View reviewed changes

dhower-qc requested changes Apr 29, 2025

View reviewed changes

arch/ext/Zclsd.yaml Show resolved Hide resolved

Shehrozkashif requested review from ThinkOpenly and dhower-qc May 8, 2025 20:07