Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup ID stage and decoder #120

Merged
merged 12 commits into from Jul 9, 2019

Conversation

@vogelpi
Copy link
Contributor

commented Jul 3, 2019

This PR contains a major cleanup of the ID stage and the decoder. Control logic (e.g. for stalls) that depends not on the decoder exclusively, is moved out of the decoder. Similarly, logic inside the ID stage that is strictly related to instruction decoding (immediate & register file address generation and checking) is moved into the decoder.

As a result, the ID stage now instantiates the modules of the controller, decoder and registers, and contains the the main muxes, the write-back (WB) FSM to handle multicycle instructions and minimal glue logic, only.
This will ease future work on any of these parts.

Finally, the WB FSM inside the ID stage is cleaned up to remove circular dependencies between FSM and decoder. This resolves #103 .

Move logic to ignore decoder output out of decoder into ID stage
The decoder shall decode the instruction only. The handling of stalls
is not related to instruction decoding.
@imphil
Copy link
Collaborator

left a comment

Thanks @vogelpi for this massive change, looks great and solves real-world problems. I've left a couple comments in the code, let me know what you think.

rtl/ibex_controller.sv Show resolved Hide resolved
rtl/ibex_id_stage.sv Outdated Show resolved Hide resolved
rtl/ibex_decoder.sv Outdated Show resolved Hide resolved
rtl/ibex_controller.sv Outdated Show resolved Hide resolved
assign instr_valid_clear_o = id_ready_o | halt_id;

// signal that ID stage has valid output
assign id_valid_o = id_ready_o & ~halt_id;

This comment has been minimized.

Copy link
@imphil

imphil Jul 8, 2019

Collaborator

It's unexpected for a valid/ready interface to have dependencies on each other.

In this case, it's probably fine, as valid and ready to in different directions (ready goes to the previous stage, valid to the next stage). Can we reflect these different directions more clearly in the signal name? (e.g. id_in_ready, id_out_valid).

We could probably go even further and rename ~halt_id to id_in_valid, and then we have a complete valid/ready interface.

This comment has been minimized.

Copy link
@vogelpi

vogelpi Jul 9, 2019

Author Contributor

Yes I totally agree. I will also include this in the same future PR that cleans up the controller and its FSM.

rtl/ibex_id_stage.sv Outdated Show resolved Hide resolved
rtl/ibex_id_stage.sv Outdated Show resolved Hide resolved
rtl/ibex_defines.sv Show resolved Hide resolved
rtl/ibex_id_stage.sv Outdated Show resolved Hide resolved
rtl/ibex_decoder.sv Outdated Show resolved Hide resolved

vogelpi added some commits Jul 1, 2019

Prevent illegal instructions from propagating out of decoder
This commit makes sure that if any instruction is detected as being
illegal inside the decoder, the decoder does not set the control
signals to let the illegal instruction affect the register file,
LSU, EX, WB, CSRs. Previously, this was only the case for some but
but not all instructions.

Note that this is not sufficient to prevent instructions detected
as illegal elsewhere from affecting the processor state. For example,
when using RV32E, an instruction can be detected to use unavailable
registers outside the decoder in the ID stage. But it is cleaner to
handle all illegal instructions detected in the decoder similarly.
Rework interaction between EX block and ID stage
The EX block actually signals when its output is valid, and not when it
is ready to accept new input. The LSU valid signal is not needed inside
the EX block and can thus be fed directly to the ID stage.
Remove decoder MUX signals for jump and branch
These signals do not need to be generated by the WB FSM inside the ID
stage and be fed back into the decoder. They simply depend on whether
the instruction is new (we execute for the first cycle) or not.

@vogelpi vogelpi force-pushed the vogelpi:cleanup-id-stage-and-decoder branch from 351e707 to a6fb12c Jul 8, 2019

@imphil

imphil approved these changes Jul 8, 2019

Copy link
Collaborator

left a comment

LGTM with the decode_we rename coming in a subsequent PR.

vogelpi added some commits Jul 3, 2019

Rework ID stage and decoder
This commit moves logic directly related to the decoder from the ID
stage into the decoder. This logic includes:
- Generation of immediates and decoder-based mux selectors
- Generation of register file addresses
- CSR operand check and manipulation depending on value in `rs1`
- Register file address check for RV32E (still disabled)

The muxes themselves stay in the ID stage as their control signals also
depend also on other, non-decoder-based signals (LSU, EX, WB FSM).

@vogelpi vogelpi force-pushed the vogelpi:cleanup-id-stage-and-decoder branch from a6fb12c to c98359a Jul 8, 2019

@vogelpi vogelpi merged commit c84ca25 into lowRISC:master Jul 9, 2019

@vogelpi vogelpi deleted the vogelpi:cleanup-id-stage-and-decoder branch Jul 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.