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

Idiv lzc opt #621

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Conversation

kaamakshee
Copy link

@kaamakshee kaamakshee commented Dec 16, 2022

This PR contains -

  • Working code for unsigned division with lzc optimization. Both bits_per_iter=1 and bits_per_iter=2 cases are addressed.

  • Updated Makefile flow:

  1. The expected output files were static and we had to change the c file each time to get a new file
  2. Automated it to just give defines (WIDTH, ITERS) as inputs to the make command
  3. Eg) After running simulation, run make test , followed by make verify
  4. make test generated new expected output files

New design doc with notes - https://docs.google.com/presentation/d/1jtqCfIULkc9yHW3kGDidr9mmkZZK64JpkwFONPGzuxM/edit#slide=id.g21dc5dfbb2f_0_521

Copy link
Contributor

@dpetrisko dpetrisko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good first iter!

bsg_misc/bsg_idiv_iterative.v Outdated Show resolved Hide resolved
bsg_misc/bsg_idiv_iterative.v Outdated Show resolved Hide resolved
bsg_misc/bsg_idiv_iterative.v Show resolved Hide resolved
,.num_zero_o(clz_a_result)
);

assign div_shift = (zero_divisor_i) ? width_p-1 : clz_a_result - clz_c_result;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should truncate to num_bits_per_iter granularity

bsg_misc/bsg_idiv_iterative_controller.v Outdated Show resolved Hide resolved
bsg_misc/bsg_idiv_iterative_controller.v Outdated Show resolved Hide resolved
bsg_misc/bsg_idiv_iterative_controller.v Outdated Show resolved Hide resolved
bsg_misc/bsg_idiv_iterative_controller.v Outdated Show resolved Hide resolved
Copy link
Contributor

@dpetrisko dpetrisko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great; just need to fix up for 2b at a time

dpetrisko pushed a commit that referenced this pull request Jan 26, 2023
@@ -22,6 +22,8 @@ module bsg_idiv_iterative_controller #(parameter width_p=32, parameter bits_per_
,input opA_is_neg_i
,input opC_is_neg_i

,input [`BSG_SAFE_CLOG2(width_p)-1:0] div_shift
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; coding convention is div_shift_i

@@ -38,6 +40,8 @@ module bsg_idiv_iterative_controller #(parameter width_p=32, parameter bits_per_
,output logic latch_signed_div_o
,output logic adder1_cin_o

,output logic [`BSG_WIDTH(width_p):0] shift_val
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; coding convention is shift_val_o

@@ -38,6 +40,8 @@ module bsg_idiv_iterative_controller #(parameter width_p=32, parameter bits_per_
,output logic latch_signed_div_o
,output logic adder1_cin_o

,output logic [`BSG_WIDTH(width_p):0] shift_val
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The max value of this seems to be 'BSG_WIDTH(width_p) as it's assigned to width_p or div_shift (which has max value of width_p). I think this port is 1b too wide (or am I misreading?)

@@ -22,6 +22,8 @@ module bsg_idiv_iterative_controller #(parameter width_p=32, parameter bits_per_
,input opA_is_neg_i
,input opC_is_neg_i

,input [`BSG_SAFE_CLOG2(width_p)-1:0] div_shift

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add link to Google design doc in header comment

@taylor-bsg
Copy link
Contributor

Do we have a modified schematic for the changes?

I think the support for these changes should be based on an input parameter, so the user can select whether they want to pay the area overhead

@kaamakshee
Copy link
Author

Looks great; just need to fix up for 2b at a time

Changes made for 2b case. Cleaning up code now. Also, the testing flow is static. Working on making it easier by making makefile updates.

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.

3 participants