-
Notifications
You must be signed in to change notification settings - Fork 10
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
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
|
|
||
| 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_twhen we are sure the wire will be used to carry exclusively instructions, such as input to instruction decode;addr_twhen we are sure the wire is used to carry exclusively address (e.g.pc)data_tfor 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 |
|
This was included into |
Extended #12 with unconditional branch support via
pc_srcsignal from the control FSM.