Sivaraman-style PIFO #81
Conversation
- insert_idx_C is now [N:0] wide - passes single push/pop per cycle tests - (very) broken on dual pushes
- change file structure - clarify testbench outputs
- use packed arrays in both the flow scheduler and rank store - Store ranks in the rank store (I am highly dum)
anshumanmohan
left a comment
anshumanmohan
left a comment
There was a problem hiding this comment.
Super cool, and what a remarkable achievement! From zero .sv to all this good stuff! As I said, I have reviewed it mostly just to learn. My comments are all requests for more commentary and hand-holding. This is especially relevant because we are not necessarily a verilog-heavy lab, and so a future ugrad or PhD student who may need to modify this code could get super spooked! Please see the spots that I have marked, and please also take a look at the rest of the code yourself and add in some comments. Use verbose mode, as it were. Comment it up for a pre-summer, un-caffeinated version of yourself.
| input clk, | ||
| input rst, | ||
|
|
||
| // For enqueue |
There was a problem hiding this comment.
Consider adding a quick little comment explaining why we have two sets of inputs for push?
| always_comb begin | ||
| for ( int j = 0; j < 2; j++ ) begin | ||
| insert_idx_C[j][SIZE] = 1; | ||
| for ( int i = 0; i < SIZE; i++ ) | ||
| insert_idx_C[j][i] = !valids[i] || push_rank_C[j] < ranks[i]; | ||
| end | ||
|
|
||
| for ( int c = 0; c < 2; c++ ) begin | ||
| for ( int s = 1; s >= 0; s-- ) | ||
| if ( push_valid_S[s] && | ||
| ( insert_idx_S[s] > insert_idx_C[c] || | ||
| ( insert_idx_S[s] == insert_idx_C[c] && push_rank_S[s] <= push_rank_C[c] ))) | ||
| insert_idx_C[c] = insert_idx_C[c] << 1; | ||
|
|
||
| if ( pop_valid_S && !insert_idx_C[c][0] ) | ||
| insert_idx_C[c] = insert_idx_C[c] >>> 1; | ||
| end | ||
| end |
There was a problem hiding this comment.
Sorry, I just need a little more commentary here to understand what's up
| logic [1:0] push_valid_S; | ||
| logic [SIZE:0] insert_idx_S [1:0]; |
There was a problem hiding this comment.
Genuinely just curious: does this difference in whitespace have a semantic meaning? Or perhaps an idiomatic meaning in the community?
| // swap if | ||
| // 1) both pushes are valid and push_2's rank < push_1's rank | ||
| // 2) push_2 is valid but push_1 isn't |
There was a problem hiding this comment.
A bit confused.
- I assume the swap happens if either condition 1 or condition 2 is met. Please clarify in the comment.
- The logic just below does not seem to obviously be aligned with the comment. Is it just fancy boolean footwork?
- Forgive me, but I could also use some more comments inline to explain what is happening and what is being swapped. At first glance it looks to me like the if and the else branches both trigger swaps?
| do_rs_push = 0; | ||
| do_fs_push_2 = 0; | ||
| for ( int i = 0; i < FLOWS; i++ ) begin | ||
| do_rs_push = do_rs_push || ( ( flow_non_empty[i] || ( fs_push_1 && fs_push_flow_1[i]) ) && push_flow[i] ); |
There was a problem hiding this comment.
| do_rs_push = do_rs_push || ( ( flow_non_empty[i] || ( fs_push_1 && fs_push_flow_1[i]) ) && push_flow[i] ); | |
| do_rs_push = do_rs_push || ( ( flow_non_empty[i] || ( fs_push_1 && fs_push_flow_1[i]) ) && push_flow[i] ); |
There was a problem hiding this comment.
Truly a minuscule thing, but you could spend a minute seeing if there is something like Black but for .sv and .cpp, and then apply those formatters all around.
|
@anshumanmohan Totally agree with all your comments: most of this code needs way more exposition to be understandable. I'll certainly add comments addressing the parts you highlight, but, maybe I can TeX up a separate document to thoroughly explain the design? That way I can draw diagrams and FSMs! What do you think? |
|
Yes for sure, write up something separate! That's a great idea. The choice of format is not very important at all, but if you ask me I'd say: use TeX if you think you'd benefit from the additional features; use Markdown if you think it's good enough. Having it in an immediately-readable format like Markdown will let it live on Github or Calyx docs, and will lower the barrier to entry just a tiny bit. But truly don't worry about the choice for format much at all |
This PR builds the Verilog for Sivaraman et al.'s PIFO design and an associated Verilator testbench!