Round Robin oracle and eDSL #2177
Conversation
There was a problem hiding this comment.
Nice stuff, Cassandra!! Congratulations on really really moving the needle on our PIFOs! I have a bunch of comments for you here, but I think you will find they are all easy to understand and implement. Largely I am just trying to get your code to be future-proof and readable.
I have only reviewed one of your eDSL .py files. I don't think three of those should be merged; it's quite bad for code maintainability. Let's talk more IRL about the fix for this! But in the meantime please make fixes to that one file only; don't bother trying to massage all three files into shape. We won't need that, as I'll show you
|
At this point does this PR totally subsume #2147? If so, please say so in the conversation of that PR, be sure to link to this PR, and close that PR. Also rename this PR so it more accurately reflects all that you've done in it. Not just the oracles but also the eDSL code to match! Hooray! |
|
In your topmatter of this PR, you give us the commands to generate .data and .expect files. Would you mind trying to put those commands into gen_queue_data_expect.sh? My bad, I should have done this with you much earlier and that would have saved you some copy-pasting. Better late than never! |
|
Also I jumped in and changed your topmatter a little: we're calling them round robin queues, not red robin! 😛 |
…ze_pifo_oracle merge to pull
|
I jumped in and made a few changes, and I think you'd enjoy working through them a little, @csziklai! I have one very concrete request in the comments for you, since I think it will greatly improve our scalability. Please catch me IRL if you'd like to work through it together! After you have taken a stab at that, I'll jump back in and do the last few things I wanted. For now, you have control :) |
There was a problem hiding this comment.
Awesome job, Cassandra!! 🎉
I jumped in and made a few more changes. I recommend you look through my commits and see what you think of them. I tried to make each commit discrete and easy to understand; they more or less contain one logical change each.
I was able to massage away the value = 0 edge case. I was also able to lift a little more code into roundrobin.py. Take a look, I think you'll like it!
One last thing before we merge: could you please remove the directory strict_queues from this branch? Something like git rm should work. You may need a -r flag since it's a directory.
| incr_hot_wraparound = cb.if_with( | ||
| # If hot = numflows - 1, we need to wrap around to 0. Otherwise, we increment. | ||
| pifo.eq_use(hot.out, numflows - 1), | ||
| pifo.reg_store(hot, 0, "reset_hot"), | ||
| pifo.incr(hot), | ||
| ) |
There was a problem hiding this comment.
I pulled out this common pattern into its own little thing! Feels like a helper function now, no? :)
|
Also just a heads up, I found it helpful to always run: so that I knew for sure that I was working with fresh .data and .expect files. In case I did see errors from and then examined the file |
|
Hooray! Now feel free to hit the big green "squash and merge" button, @csziklai! |
It looks like `calyx-py/test/correctness/queues/*.py` was removed from the path of its runt test by #2177: i.e. the old `fifo`, `pifo`, and `pifo_tree` tests aren't running at CI. Was this intentional? In case it wasn't, I've made a minor fix to `runt.toml`.
This PR makes progress towards #1810. It implements the python oracle for PIFOs generalized to n flows, now known as Round Robin queues. Just as with the PIFO, if a flow falls silent, the remaining flows will take their turns. That flow effectively skips its turn. To re-generate the test files with 20000 commands and a max length of 16, type in the command line after navigating to the directory calyx/calyx-py/calyx
Additionally, this PR also implements the Calyx version of Round Robin queues in rr_queue.py. This was originally supposed to be its own PR, but I thought it might be more complicated if I branched off a branch. To run these tests, type in the command line