Add support for Zcmp extension

[nadime15]

Following #525 and #610, this is the third attempt to add support for the Zcmp extension.

Thankfully most the work was already done and the code remains largely the same as in #610, with a few minor fixes. I was able to compile the model and successfully run most tests (details on the issue below).

I have added a set of tests to the riscv-test suite, which can be found here:

To run the tests, you will need the latest RISC-V GNU Toolchain (version 15). I had to switch to the developer branches (trunk) of binutils and GCC to compile the extension successfully. While Zcmp is officially supported from version 14, a bug currently prevents compilation with the latest official release available on GitHub.

The command to execute is:

./configure \
  --prefix=/opt/riscv \
  --target=riscv64-unknown-linux-gnu \
  --with-arch=rv64g_zcmp \
  --with-abi=lp64d \
  --with-multilib-generator="rv64g_zcmp-lp64--;rv64g_zcmp-lp64d--"

I ran into two problems which I would like to discuss here:

1. I am not able to get rid of the following error warning(s):

Warning: Incomplete pattern match statement at riscv_insts_zcmp.sail:10.0-8:
10 |function clause extensionEnabled(Ext_Zcmp) = extensionEnabled(Ext_Zca) & not(extensionEnabled(Ext_Zcd)) & (xlen == 32 | xlen == 64)
   |^------^
   | 
The following expression is unmatched: <future extension clause>

Warning: Incomplete pattern match statement at riscv_insts_end.sail:23.0-8:
23 |function clause execute C_ILLEGAL(s) = { handle_illegal(); RETIRE_FAIL }
   |^------^
   | 
The following expression is unmatched: <future ast clause>(undefined)

Warning: Incomplete pattern match statement at riscv_insts_zcmp.sail:10.0-8:
10 |function clause extensionEnabled(Ext_Zcmp) = extensionEnabled(Ext_Zca) & not(extensionEnabled(Ext_Zcd)) & (xlen == 32 | xlen == 64)
   |^------^
   | 
The following expression is unmatched: <future extension clause>

Warning: Incomplete pattern match statement at riscv_insts_end.sail:23.0-8:
23 |function clause execute C_ILLEGAL(s) = { handle_illegal(); RETIRE_FAIL }
   |^------^
   | 
The following expression is unmatched: <future ast clause>(undefined)

I am able to compile without warnings by incorporating the proposed changes from #689, but these changes have not been merged yet. Is there a way to get rid of these warnings?

This brings me to the second problem.

2. While testing the extension, I made an interesting observation.

I had been running the test suite in two different environments, one with virtual memory disabled (p) and the other with virtual memory enabled (v).

All tests without virtual memory passed. However, once I enabled virtual memory, I noticed that all Zcmp instructions were incorrectly decoded and instructions were decoded as c.fsdsp. The Zcd extension defines c.fsdsp, whose encoding overlaps with instructions of the Zcmp extension. I still don’t understand why enabling virtual memory caused this incorrect decoding.

According to the RISC-V Unprivileged Specification, Zcmp cannot be used while the Zcd extension is enabled, and I am unsure how to properly integrate Zcmp into our current workflow (specifically in ./modal/CMakeLists.txt).

I am also uncertain whether this issue is related to the first problem. Maybe someone has some ideas?

[Alasdair]

I think the logic for both extensions cannot be enabled together should be in the extensionEnabled clause something like:

extensionEnabled(Ext_Zcd) = sys_enable_zcd() & not(sys_enable_zcmp())
// and
extensionEnabled(Ext_Zcmp) = sys_enable_zcmp() & not(sys_enable_zcd())

[rez5427]

what

[rez5427]

You should at least add a co-author（

[nadime15]

Hey @rez5427, thank you for pointing that out! I didn’t mean to overlook your contribution, which is why I linked to your pull request in my PR message. I’ve now added you as a co-author in the commit.

[Alasdair]

Yes, we should make sure when rebasing older PRs to ensure the original authors are preserved with the Co-authored-by field. PR authors should also feel free to add themselves to the list of authors.

[nadime15]

@Alasdair Yeah sure, will do!

In #8950389 I added the Zcmp extension to the C emulator along with the --enable-zcmp command-line argument.

I retested everything, and it now works with or without virtual memory.

However, the warnings are still there, even after using sys_enable_zcmp() and sys_enable_zcd.

Maybe I missed something?

[jordancarlin]

However, the warnings are still there, even after using sys_enable_zcmp() and sys_enable_zcd.

Maybe I missed something?

Assuming you’re talking about the 4 warnings in your initial message, they won’t be impacted by these changes. They are new warnings introduced by the unreleased version of sail that I assume you’re using and either need changes to sail itself or #689.

[jordancarlin]

Hey @rez5427, thank you for pointing that out! I didn’t mean to overlook your contribution, which is why I linked to your pull request in my PR message. I’ve now added you as a co-author in the commit.

It looks like the co-author is messed up on the first commit because of an erroneous quotation mark.

[Alasdair]

Yes as Jordan said, those warnings are unrelated to this commit. Don't worry about them.

[github-actions]

Test Results

392 tests  ±0   392 ✅ ±0   1m 21s ⏱️ -1s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit af6889e. ± Comparison against base commit c650166.

♻️ This comment has been updated with latest results.

[nadime15]

@jordancarlin Oh yes! Good catch, I am going to fix that!

Btw. I forgot to change one line, which is fixed in 3d9cd3e

Yes exactly I use the unreleased version of sail. But now I know thanks.

[nadime15]

One aspect worth discussing: According to the spec, PUSH and POP operations can be written in any order, grouped, or even written multiple times.

Additionally, an interrupt can occur during the read/write sequence and the interrupt handler might overwrite the written bytes and we have to start the whole read/write sequence from the beginning.

For example, would it make sense to replace this block with repeated STORE calls?

function process_cmpush (rlist : bits(4), spimm : bits(2)) -> Retired = {
  let width_bytes = xlen_bytes;
  assert(width_bytes <= xlen_bytes);

  let addr = X(sp);
  let stack_adj = negate_int(get_stack_adj_base(rlist) + unsigned(spimm) * 16);
  let new_sp = addr + to_bits(xlen, stack_adj);
  let mask = zcmp_regmask(rlist);

  var offset : int = width_bytes;

  foreach (i from 31 downto 1) {
    if mask[i] == bitone then {
      match ext_data_get_addr(sp, to_bits(xlen, negate_int(offset)), Write(Data), xlen / 8) {
        Ext_DataAddr_Error(e)  => { ext_handle_data_check_error(e); return RETIRE_FAIL },
        Ext_DataAddr_OK(vaddr) =>
        if   check_misaligned(vaddr, size_bytes(xlen_bytes))
        then { handle_mem_exception(vaddr, E_SAMO_Addr_Align()); return RETIRE_FAIL }
        else match translateAddr(vaddr, Write(Data)) {
            TR_Failure(e, _)    => { handle_mem_exception(vaddr, e); return RETIRE_FAIL },
            TR_Address(paddr, _) => {
              let eares = mem_write_ea(paddr, width_bytes, false, false, false);
              match (eares) {
                  Err(e) => { handle_mem_exception(vaddr, e); return RETIRE_FAIL },
                  Ok(_) => {
                  let reg_val = X(to_bits(5, i));
                  match mem_write_value(paddr, width_bytes, reg_val[width_bytes * 8 - 1 .. 0], false, false, false) {
                      Ok(true)  => { offset = offset + width_bytes },
                      Ok(false) => internal_error(__FILE__, __LINE__, "store got false from mem_write_value"),
                      Err(e) => { handle_mem_exception(vaddr, e); return RETIRE_FAIL }
                    }
                  }
              }
            }
        }
      }
    }
  };
  X(sp) = new_sp;
  RETIRE_SUCCESS
}

replacing with

function process_cmpush (rlist : bits(4), spimm : bits(2)) -> Retired = {
  let addr = X(sp);
  let stack_adj = negate_int(get_stack_adj_base(rlist) + unsigned(spimm) * 16);
  let new_sp = addr + to_bits(xlen, stack_adj);
  let mask = zcmp_regmask(rlist);

  var offset : int = xlen_bytes;
  
  foreach (i from 31 downto 1) {
    if mask[i] == bitone then {
      let reg = to_bits(5, i);
      execute(STORE(to_bits(12, negate_int(offset)), reg, sp, size_bytes(xlen_bytes), false, false));
      offset = offset + xlen_bytes;
    }
  };
  X(sp) = new_sp;
  RETIRE_SUCCESS
}

As implemented now, it seems no interrupt can be detected during execution (?).

This raises the question: Can I even nest execution clauses? As far as I understand the simulation continues only once a instruction is either retired or has failed.

In this case, PUSH would call STORE multiple times and PUSH is not yet retired and has not yet failed

[pmundkur]

It might be better to factor out the core address logic from STORE into a separate function, and call that from STORE and process_cmpush.

If interrupts are to be taken during the loop (which is up to the implementation, according to the spec), it would need to involve the stepper. This might be easier after something like #746.

[nadime15]

Yes, I agree on both points. Having some form of access to the stepper is probably the way to go, just in case someone wants to run a test case with interrupts between load and store sequences.