Skip to content

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

TheDeepestSpace
Copy link
Contributor

@TheDeepestSpace TheDeepestSpace commented Mar 11, 2025

Extended #12 with unconditional branch support via pc_src signal from the control FSM.

@TheDeepestSpace TheDeepestSpace force-pushed the branch branch 4 times, most recently from 8cf5c97 to 4a1d5ae Compare March 27, 2025 02:43
@TheDeepestSpace TheDeepestSpace marked this pull request as ready for review March 27, 2025 02:46
Comment on lines +49 to +52
extend extend
( .in ( instr[31:20] )
, .imm_ext ( imm_ext )
);
Copy link
Contributor Author

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.

Copy link
Contributor Author

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;
Copy link
Contributor Author

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:

  1. instr_t when we are sure the wire will be used to carry exclusively instructions, such as input to instruction decode;
  2. addr_t when we are sure the wire is used to carry exclusively address (e.g. pc)
  3. 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)

Comment on lines +46 to +47
assign pc_mux_in[0] = pc_target;
assign pc_mux_in[1] = pc_plus_4;
Copy link
Contributor Author

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

Copy link
Contributor Author

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

Comment on lines +46 to +52
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 )
);
Copy link
Contributor Author

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.

@TheDeepestSpace
Copy link
Contributor Author

I poped the vcd's for the testbenches into #27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant