Add Zilsd/Zclsd Support #765

trdthg · 2025-03-04T08:49:51Z

spec: https://github.com/riscv/riscv-zilsd/blob/main/zilsd.adoc#insns-sd

zilsd:
- RV32 only
- load/store 64 bit value
- has same assembly with 2.6. Load and Store Instructions
zclsd:
- RV32 only
- load/store 64 bit value
- has same assembly with 26.3. Load and Store Instructions
- compressed
- base = rs1 or stack_pointer
- has overlapping encodings with Zcf

function clause extensionEnabled(Ext_Zclsd) = true & xxx is a placeholder, waiting for config system

github-actions · 2025-03-04T08:58:39Z

Test Results

398 tests ±0 398 ✅ ±0 1m 45s ⏱️ ±0s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 179b3be. ± Comparison against base commit 513a705.

♻️ This comment has been updated with latest results.

nadime15 · 2025-03-04T16:23:00Z

model/riscv_insts_zilsd.sail

+  match ext_data_get_addr(rs1, offset, Read(Data), width_bytes) {
+    Ext_DataAddr_Error(e)  => { ext_handle_data_check_error(e); RETIRE_FAIL },
+    Ext_DataAddr_OK(vaddr) =>
+      if   check_misaligned(vaddr, width)
+      then { handle_mem_exception(vaddr, E_Load_Addr_Align()); RETIRE_FAIL }
+      else match translateAddr(vaddr, Read(Data)) {
+        TR_Failure(e, _) => { handle_mem_exception(vaddr, e); RETIRE_FAIL },
+        TR_Address(paddr, _) =>
+          match mem_read(Read(Data), paddr, 8, false, false, false) {
+            Ok(result) => {
+              X(rd)     = result[31..0];
+              X(rd + 1) = result[63..32];
+              RETIRE_SUCCESS
+            },
+            Err(e)     => { handle_mem_exception(vaddr, e); RETIRE_FAIL },
+          },
+      }
+  }
+}


I'm curious, wouldn't it make more sense to replace LOAD_RV32 (and STORE_RV32) with two sequential 32-bit load/store operations instead of one 64-bit load/store operation?

Most 32-bit systems have a 32-bit memory bus, so performing two 32-bit accesses would be more realistic than a single 64-bit access on such hardware.

This implementation would also allow us to test the more interesting case where interrupts could occur between the two operations, as explicitly permitted by the Zilsd specification.

That's definitely a good idea, I didn't think of that

pmundkur · 2025-03-07T15:22:11Z

model/riscv_insts_zclsd.sail

+/*  SPDX-License-Identifier: BSD-2-Clause                                                */
+/*=======================================================================================*/
+
+function clause extensionEnabled(Ext_Zclsd) = true & extensionEnabled(Ext_Zilsd) & extensionEnabled(Ext_Zca) & not(extensionEnabled(Ext_Zcf)) & xlen == 32


This seems to create a mutual cycle between extensionEnabled(Ext_Zclsd) and extensionEnabled(Ext_Zcf).

After the configuration system merged, update to config extensions.Zclsd.enabled should resolve the issue.

pmundkur · 2025-03-07T15:36:12Z

model/riscv_insts_zilsd.sail

+function clause extensionEnabled(Ext_Zilsd) = true & xlen == 32
+
+/* ****************************************************************** */
+union clause ast = LOAD_RV32 : (bits(12), regidx, regidx)


LOAD_DOUBLE might be a better name for the AST constructor than LOAD_RV32. It's unfortunate that the spec re-uses LOAD. (Could we ask the spec authors to change this before ratification?) Similarly for STORE_RV32.

I saw your reply at riscv/riscv-zilsd#64, so I update it to the format of EXT_INST(ZILSD_LD...).

pmundkur · 2025-03-07T15:36:16Z

model/riscv_insts_zilsd.sail

+
+function clause execute LOAD_RV32(imm, rs1, rd) = {
+  // Use of misaligned (odd-numbered) registers is reserved.
+  assert(regidx_to_regno(rd) % 2 == 0);


Instead of asserting here, it would be better to put this constraint on rd in the encdec clause. Similarly for rs2 in the store instruction.

christian-herber-nxp · 2025-03-07T16:06:08Z

how does this relate to #490?

trdthg · 2025-03-10T01:22:35Z

how does this relate to #490?

I didn't see it before. Currently, the code mainly has the following differences

zcmlsd -> zclsd
directly concat the 0 at the end of the imm to the ast
ast naming inconsistency (adopted theirs, I haven't added co-authored-by yet because I don't know his email)
simply execute
TODO some refactor based on the new features of sail
- Update mappings to use the when construct for guards #768
- Replace ad-hoc model configuration with Sail-native configuration #758

pmundkur · 2025-03-10T15:02:44Z

@trdthg Could you check with @christian-herber-nxp to make sure we're not missing anything from #490 and we get the appropriate co-authors?

christian-herber-nxp · 2025-03-10T17:09:53Z

the author of the original PR should be clear from that, it is @simonacostinescu

trdthg · 2025-03-24T02:47:38Z

Update

I am trying to run act tests (there is already a PR, not merged yet), but encountered a few problems

riscv-config Updating Load/Store Pair for RV32 to version 0.10 riscv-software-src/riscv-config#189 waiting for merge
~~the tests have some issue(seems about ISA constrains), I'm doing some fix~~ done
gcc zilsd/zclsd support not merged yet(and it's not complete, some orgs have complete version, but lack maintenance, need rebase), trying with clang / compile gcc locally

I think you could review this after I finished those tests.

Co-authored-by already added

One of the tricky parts with this extension is that it is designed to have its instructions overridden by other extensions (like `Zicfiss`). This will be handled by relying on file order to override instructions. This is a little fragile with the current compilation method, where all `.sail` files are passed to the compiler. In future we will use a Sail project file which allows enforcing file order more robustly.

Co-authored-by: Simona Costinescu <[email protected]>

jordancarlin · 2025-04-08T12:32:47Z

model/riscv_insts_zilsd.sail

+function clause execute ZILSD_LD(imm, rs1, rd) = {
+  if rd != zreg then {
+    let base_val = X(rs1);
+    let _ = load_imm(imm, base_val, rd, WORD);


Won't this cause issues if the first load fails? I think you need to check what it returns so that we get the RETIRE_FAIL if the first load fails.

Right now if the first load fails but the second one succeeds (like if it is right on some memory region boundary) this instruction will act like it succeeded.

trdthg force-pushed the zilsd branch from 22c1fb9 to 9687d88 Compare March 4, 2025 08:51

nadime15 reviewed Mar 4, 2025

View reviewed changes

nadime15 mentioned this pull request Mar 4, 2025

Create generalized interface for repetitive load/store instructions #766

Open

trdthg force-pushed the zilsd branch 2 times, most recently from a979234 to a28bd1b Compare March 5, 2025 01:02

jordancarlin added the extension Adds support for a RISC-V extension label Mar 5, 2025

pmundkur requested changes Mar 7, 2025

View reviewed changes

pmundkur mentioned this pull request Mar 7, 2025

Reuse of LOAD/STORE in instruction names is causing an issue in the Sail RISC-V model riscv/riscv-zilsd#64

Closed

trdthg force-pushed the zilsd branch from a28bd1b to f83324b Compare March 10, 2025 01:12

trdthg force-pushed the zilsd branch 2 times, most recently from d6533f5 to d70a272 Compare March 10, 2025 01:30

pmundkur added the tgmm-agenda Tagged for the next Golden Model meeting agenda. label Mar 10, 2025

trdthg force-pushed the zilsd branch 2 times, most recently from c1bff11 to ea18ab9 Compare March 24, 2025 02:43

jordancarlin removed the tgmm-agenda Tagged for the next Golden Model meeting agenda. label Apr 7, 2025

jordancarlin and others added 2 commits April 8, 2025 15:49

Add Zilsd/Zclsd Support

179b3be

Co-authored-by: Simona Costinescu <[email protected]>

trdthg force-pushed the zilsd branch from ea18ab9 to 179b3be Compare April 8, 2025 12:19

jordancarlin reviewed Apr 8, 2025

View reviewed changes

trdthg mentioned this pull request Apr 9, 2025

Add Zilsd/Zclsd support riscv-non-isa/riscv-arch-test#636

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Zilsd/Zclsd Support #765

Add Zilsd/Zclsd Support #765

trdthg commented Mar 4, 2025 •

edited

Loading

github-actions bot commented Mar 4, 2025 •

edited

Loading

nadime15 Mar 4, 2025

trdthg Mar 5, 2025

pmundkur Mar 7, 2025

trdthg Mar 10, 2025

pmundkur Mar 7, 2025

trdthg Mar 10, 2025

pmundkur Mar 7, 2025

trdthg Mar 10, 2025

christian-herber-nxp commented Mar 7, 2025

trdthg commented Mar 10, 2025 •

edited

Loading

pmundkur commented Mar 10, 2025

christian-herber-nxp commented Mar 10, 2025

trdthg commented Mar 24, 2025 •

edited

Loading

jordancarlin Apr 8, 2025

Add Zilsd/Zclsd Support #765

Are you sure you want to change the base?

Add Zilsd/Zclsd Support #765

Conversation

trdthg commented Mar 4, 2025 • edited Loading

github-actions bot commented Mar 4, 2025 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-herber-nxp commented Mar 7, 2025

trdthg commented Mar 10, 2025 • edited Loading

pmundkur commented Mar 10, 2025

christian-herber-nxp commented Mar 10, 2025

trdthg commented Mar 24, 2025 • edited Loading

Update

Choose a reason for hiding this comment

trdthg commented Mar 4, 2025 •

edited

Loading

github-actions bot commented Mar 4, 2025 •

edited

Loading

trdthg commented Mar 10, 2025 •

edited

Loading

trdthg commented Mar 24, 2025 •

edited

Loading