[XIF] Coprocessor XIF Issue response not sampled by ID-EX pipeline registers

Placeholder Michele Caon
2023-09-08T05:36:35Z

## Coprocessor XIF Issue response not sampled by ID-EX pipeline registers
The `cv32e40x` CPU attempts to offload an instruction to a coprocessor connected through the CORE-V eXtension Interface (XIF `issue_req`) whenever its instruction decoder fails to recognize it (or when the instruction is a CSR instruction).

Currently, instruction offloading in the decode stage (ID) does *not* check for backpressure from the CPU execution stage (EX) when sending the request (`xif_issue.issue_valid = 1'b1`), possibly resulting in the feedback information from the coprocessor response (e.g., the intention to write back the offloaded instruction result to one of the CPU scalar GPRs `x0`-`x31`) not being sampled by the ID-EX pipeline register and, therefore, not being propagated to the following stages.

This isssue is potentially already known, as it seems to be related to the comment at [`cv32e40x_id_stage.sv:752`](https://github.com/openhwgroup/cv32e40x/blob/1c7c5e6d1736dceeacd079b10cf6b3a70ad857b4/rtl/cv32e40x_id_stage.sv#L752), but the example provided below may help in debugging and solving it.

The very simple workaround we implemented (described below) is suboptimal, as it delays XIF instruction offloading until EX is ready to accept a new instruction (0 or 1 cycles in most cases). Therefore, no pull request was opened, although I am available to provide further support if needed.

### Component
**RTL**: `rtl/cv32e40x_id_stage.sv`

### Steps to Reproduce
The example below was simulated on revision [`f17028f`](https://github.com/openhwgroup/cv32e40x/commit/f17028f2369373d9443e4636f2826218e8d54e0f) (`v0.9.0`).

#### Assembly code
The CPU execution stage applies backpressure to the instruction decoding stage (i.e., `ex_ready_i = 0`) whenever the instruction execution takes more than one cycle, which is the case, for example, for memory accesses (`load`/`store` instructions). We triggered this situation with the following code snippet:
```asm
...
lw t0, CTL_REG_VL_REG_ADDR(zero)
lw t1, CTL_REG_VTYPE_REG_ADDR(zero)
sw zero, CTL_REG_OP_CTL_REG_ADDR(zero)
vsetvl t0, t0, t1 # instruction offloaded to the coprocessor through XIF
sw t0, CTL_REG_VL_REG_ADDR(zero) # write offloaded instr. result to `t0`
lw a0, CTL_REG_ARG0_REG_ADDR(zero) # load arg0 from control registers
...
```
Where `vsetvl` (from the RISC-V "V" vector extension) is an instruction to be offloaded to a vector coprocessor using the CORE-V eXtension Interface (XIF). This is a setup instruction that attemps to write the value in the source GPR (`t0` here) to a CSR in the coprocessor and write the legal value actually accepted by the coprocessor to the `rd` GPR (`t0` again here).

#### Issue summary
In our case, the coprocessors instruction decoding stage can answer an offloading request in the same cycle it issued by the CPU, and the response data is retired the cycle after. In the example proposed here, the CPU execution stage is not ready to accept a new instruction when the `vsetvl` instruction is offloaded, becasue it is still processing the previous `lw`. Therefore, the coprocessor response data does *not* get sampled in the `ID-EX` pipeline register until the next cycle, _when it is no longer valid_.

The following timing diagram shows a simplified representation of what is happening with the current RTL:

![plain](https://github.com/openhwgroup/cv32e40x/assets/39236437/9498778f-e51f-4350-a377-1a38dd0238d4)

While the following timing diagram shows the expected behaviour, obtained by delaying the offload request until EX is ready (further details below):

![expected](https://github.com/openhwgroup/cv32e40x/assets/39236437/4d5c9fe8-ade9-47eb-8471-dd1efce455ba)

#### Waveforms and details
The waveforms dumped during simulation and the GTKWave view configuration files can be found in the attached archive:

[cv32e40x-issue.zip](https://github.com/openhwgroup/cv32e40x/files/12549848/cv32e40x-issue.zip)

The XIF issue transaction from the example above begins at `33144ns` in the attached VCD file `plain.vcd`, that can be opened with:
```bash
gtkwave -a plain.gtkw plain.vcd
```

As shown, the `issue_valid` signal from the CPU and the `issue_ready` and `accept` signals from the coprocessor are active in the same cycle (marker `A`), indicating that the offloading transaction succeded. The coprocessor also communicate that the instruction can trigger an exception (`xif_exception`) and that it will write back the instruction result in one of the CPU GPRs (`t0` in particular). In this cycle, the CPU EX stage is still busy executing the previous `lw` instruction (`ex_ready_i = 0` in ID stage).

In the next cycle (marker `B`), the CPU EX stage becomes ready agains, so the information about the offloaded instruction that comes from the CPU IF-ID pipeline register gets sampled correctly in the ID-EX pipeline register, and the instruction is actually propagated through the CPU stages. However, the information in the coprocessor issue response, that was valid in the previous cycle when the offloading transaction took place, is missed. One of the consequences is that the result data from the coprocessor (result transaction at marker `C`) does not get written back to the CPU GPRs (`x5` doesn't change to `0x100` at marker `D`). More sever consequences may appear when the coprocessor uses the CPU load-store unit or the bus, possibly triggering errors or exceptions that would be ignored by the CPU.

### Workaround
A quick and dirty solution to get the information from the coprocessor correctly sampled into the ID-EX stage is to attempt instruction offloading through the XIF only when the execution stage is ready to accept a new instruction (i.e., `ex_ready_i = 1` in ID stage). We implemented this by making the `issue_valid` XIF signal depend on the `ex_ready_i` signal from the execution stage, as shown in the following patch:
```
diff --git a/rtl/cv32e40x_id_stage.sv b/rtl/cv32e40x_id_stage.sv
index 1385dfb..d760e55 100644
--- a/rtl/cv32e40x_id_stage.sv
+++ b/rtl/cv32e40x_id_stage.sv
@@ -749,7 +749,9 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
// Also attempt to offload any CSR instruction. The validity of such instructions are only
// checked in the EX stage.
// Instructions with deassert_we set to 1 from the controller bypass logic will not be attempted offloaded.
- assign xif_issue_if.issue_valid = instr_valid && (illegal_insn || csr_en) &&
+ // Only offload instructions if the EX stage is ready not to miss data from xif_issue.issue_resp
+ assign xif_issue_if.issue_valid = instr_valid && ex_ready_i &&
+ (illegal_insn || csr_en) &&
!(xif_accepted_q || xif_rejected_q || ctrl_byp_i.deassert_we);

// Keep xif_offloading_o high after an offloaded instruction was accepted or rejected to get
```

This produced the expected result included in the `patched.vcd` dump, that can be opened with:
```bash
gtkwave -a patched.gtkw patched.vcd
```

Here, the XIF offload transaction is delayed by one cycle and takes place at `33146ns` (marker `A`), when the EX stage is ready. This way, the response data from the coprocessor get sampled correctly in the ID-EX pipeline register (marker `B`). As a consequence, the result provided by the coprocessor (at marker `C`) gets correctly written back to the target GPR (`x5` value changes from `0x101` to `0x100` at marker `D`).

> :warning: **NOTE** : *this is not an optimal solution*, as it effectively stalls the coprocessor for one cycle (possibly more if the previous instruction has a longer execution latency). A better way to fix this issue may be to offload an instruction to the coprocessor as soon as it is fetched and decoded (as it is now without the patch), and sample the coprocessor response data in a dedicated register (different from the ID-EX pipeline register) while waiting for the execution stage to be ready. Still, I'm not sure whether there are implications of such solution on the synchronization between the coprocessor and the CPU, so I chose the safer, unoptimized way.

Please let me know if further details are needed.

Thank you for your attention and collaboration,
Michele