prototype shifter done #1
Conversation
coderkalyan
left a comment
coderkalyan
left a comment
There was a problem hiding this comment.
Thanks for the work! Left a few initial comments.
| // `XARITH_OP_SLT: o_result = (i_op1 < i_op2) ? 1'b1 : 1'b0 | ||
| // `XARITH_OP_CMP: o_result = min/max(i_op1, i_op2) | ||
| input wire [1:0] i_opsel, | ||
| input wire [ 1:0] i_opsel, |
There was a problem hiding this comment.
Autoformatting definitely needs to be done but please don't add it here in this PR.
There was a problem hiding this comment.
ye thats a next time problem my bad
rtl/warp_integer.v
Outdated
|
|
||
| // comparison | ||
| wire lt = sum[63]; | ||
| wire lt = sum[63]; |
There was a problem hiding this comment.
Same here and everywhere else. Also, the equals signs were explicitly aligned here so the formatter needs to be tweaked.
rtl/warp_integer.v
Outdated
| begin | ||
| // implement bypassing for all read ports, assumes | ||
| // rd1 != rd2 | ||
| if (i_rd1_wen && (i_rs1_addr == i_rd1_addr)) |
There was a problem hiding this comment.
Yeah please avoid especially these large changes to unrelated modules.
tb_shifter.v
Outdated
| $dumpvars(0, tb_shifter); | ||
| end | ||
|
|
||
| wire i_word_true = i_word == 1'b1; |
There was a problem hiding this comment.
i_word_true is the same as i_word and i_word_false is the same as !i_word, more readable
tb_shifter.v
Outdated
| wire i_word_true = i_word == 1'b1; | ||
| wire i_word_false = i_word == 1'b0; | ||
|
|
||
| initial begin |
There was a problem hiding this comment.
Take a look at the formal article I sent, should be using an always @(*) combinational block here.
rtl/warp_integer.v
Outdated
| input wire i_rst_n, | ||
| input wire [63:0] i_operand, | ||
| input wire [5:0] i_amount, | ||
| input wire i_clk, |
There was a problem hiding this comment.
So the interface to the shifter should be sequential. It's fine if you want to keep it single cycle for now (but update the comments if you do). I don't see any pipeline registers here to latch o_result.
rtl/warp_integer.v
Outdated
| assign stage5_ror = (i_amount[5]) ? ({stage4_ror[31:0], stage4_ror[63:32]}) : stage4_ror; | ||
|
|
||
| // Select operation for final output | ||
| assign o_result_tmp = |
There was a problem hiding this comment.
No correctness issues here but please use a case statement in these cases. Ternary is great for the above stages, but for a multi-way mux between outputs it may synthesize better to a flatter tree than the ternary chain.
rtl/warp_integer.v
Outdated
| stage5_ror; | ||
|
|
||
| // Stage 0: rotate by 1 (32 bit) | ||
| assign stage0_32_rotate = (i_amount[0]) ? ( |
There was a problem hiding this comment.
For now I think its best to unroll these like the others. This optimizes for latency and we can do a pass later to try to reduce the area cost of the shifter. It will be large.
jwang2672
force-pushed
the
jw_multiply_shift
branch
5 times, most recently
from
c6e74df to
d1eecd4
Compare
jwang2672
force-pushed
the
jw_multiply_shift
branch
from
d1eecd4 to
87f2ce6
Compare
2 participants
its not done yet