Matrix multiply practice (Do not actually merge this) #2426
Conversation
There was a problem hiding this comment.
Great work, congrats on writing your first Calyx program 🎉 As mentioned in the review, great work on using multi-component programs :)
I made a couple of small comments but otherwise everything is looking good! Biggest nitpick is to please clean up the old commented-out code, but no problem especially since we made the PR together on the spot :) Please let me know if you have any questions!!
| "tests/matrix_multiply_tests/*.futil" | ||
| ] | ||
| cmd = """ | ||
| fud e --to dat --through icarus-verilog {} -s verilog.data {}.data |
There was a problem hiding this comment.
It seems like the CI (continuous integration) tests are failing because your .expect file contains standard error. To disable this, please add -q to the command! (The other fud-based test commands also have this)
| ] | ||
| } | ||
| } | ||
| ---STDERR--- |
There was a problem hiding this comment.
This is the standard error mentioned in the comment in runt.toml!
|
|
||
| // Modified from pass-in-register.futil | ||
| component multiply_and_add( | ||
| // a: 32, |
There was a problem hiding this comment.
Please clean up comments!
| import "primitives/binary_operators.futil"; | ||
| import "primitives/unsynthesizable.futil"; | ||
|
|
||
| // Modified from pass-in-register.futil |
There was a problem hiding this comment.
We talked a bit about pass-in-register.futil, but I just wanted to reiterate that I appreciate that you're learning from existing Calyx code and you've credited the original file! 🙂
| component main() -> () { | ||
| cells { | ||
| // 4×4 matrix memories | ||
| @external arr_1 = comb_mem_d2(32, 4, 4, 4, 4); |
There was a problem hiding this comment.
Sorry, I may have mentioned comb_mem_d* in one of our previous meetings, but it's actually better to use seq_mem_d* instead! More details in the docs :)
| val_b = std_reg(32); | ||
|
|
||
| // Multiply-and-accumulate unit | ||
| mul_add = multiply_and_add(); |
There was a problem hiding this comment.
Kudos on having your first Calyx program be a multi-component program 🎉
| read_data[done] = val_b.done; | ||
| } | ||
|
|
||
| // group update_output { |
There was a problem hiding this comment.
Please clean up unused code here and below as well!
| } | ||
|
|
||
| // Loop Increment | ||
| group updI<"promotable"=1> { |
There was a problem hiding this comment.
Small nitpick, but we use snake case for groups! (i.e. I'd change this to upd_i)
| begin_acc; // Reset accumulator | ||
| while le2.out with cond2 { | ||
| seq { | ||
| // update_acc; |
There was a problem hiding this comment.
Please clean up the comments here! (or rearrange them to comment on the groups/invokes you have in your code)
| @@ -0,0 +1,26 @@ | |||
| { | |||
| "arr_1": { | |||
| "data": [ [5, 3, 2, 6], [4, 9, 2, 1], [3, 7, 2, 6], [6, 1, 4, 2] ], | |||
There was a problem hiding this comment.
You may want to consider formatting this file so it looks like the .expect file for the sake of readability and easy comparison! Basically you want to format this as a JSON file, so if you have a formatter you just need to tell your formatter that the file is (despite its extensions) actually a JSON file.
Implemented 4 by 4 matrix multiplication in Calyx. Added runt tests. To run tests run "runt -i "Matrix_multiply correctness tests""