-
Notifications
You must be signed in to change notification settings - Fork 8
Unconditional branch support #20
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
base: main
Are you sure you want to change the base?
Conversation
8cf5c97
to
4a1d5ae
Compare
extend extend | ||
( .in ( instr[31:20] ) | ||
, .imm_ext ( imm_ext ) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this module is not really needed since instruction decode module already implements this, but does not expose the ImmExt
; i think we can keep this as is for now though, I will remove this during beq
implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed extend
in #27
@@ -6,5 +6,6 @@ | |||
|
|||
typedef logic [`PROCESSOR_BITNESS-1:0] instr_t; | |||
typedef logic [`PROCESSOR_BITNESS-1:0] addr_t; | |||
typedef logic [`PROCESSOR_BITNESS-1:0] imm_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is somewhat unnecessary. i think there should be instr_t
, addr_t
and data_t
; these exist exclusively to enhance readability and do not really carry functional meaning. The idea is to use:
instr_t
when we are sure the wire will be used to carry exclusively instructions, such as input to instruction decode;addr_t
when we are sure the wire is used to carry exclusively address (e.g.pc
)data_t
for all other cases (e.g. sending data back to register fire after computation -- can be both address and instruction, as well as mathematical result)
assign pc_mux_in[0] = pc_target; | ||
assign pc_mux_in[1] = pc_plus_4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this is the other way around -- increment should be on 0
and jump on 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fixed on #27
assign pc_mux_in[0] = pc_target; | ||
assign pc_mux_in[1] = pc_plus_4; | ||
mux #( .INPUT_COUNT ( 2 ), .INPUT_WIDTH ( `PROCESSOR_BITNESS ) ) pc_mux | ||
( .sel ( cfsm__pc_src ) | ||
, .in ( pc_mux_in ) | ||
, .out ( pc_next ) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also considering a case
statement here instead of mux
module. Would also like to use constants for 0
and 1
cases, for better readability.
I poped the vcd's for the testbenches into #27 |
Extended #12 with unconditional branch support via
pc_src
signal from the control FSM.