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.

Sign up for GitHub

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

Jump to bottom

Add matrix_multiplier to examples. #1502

Merged
merged 1 commit into from
Sep 9, 2020
Merged

Add matrix_multiplier to examples. #1502

merged 1 commit into from
Sep 9, 2020

Conversation

marlonjames
Copy link
Contributor

@marlonjames marlonjames commented Mar 17, 2020 •
edited by eric-wieser

Add matrix multiplication example and test. The test is heavily
weighted towards use of the BinaryValue class. By fixing the random
seed by default, it also provides a deterministic test for performance
regression.

This example and test are designed to be a testbench for various
performance enhancements, especially involving BinaryValue.

Split from gh-1286

@eric-wieser
Copy link
Member

There were some unaddressed comments in #1286 that would be good to apply here, I think.

ktbarrett
ktbarrett reviewed Mar 17, 2020
View reviewed changes
Copy link
Member

@ktbarrett ktbarrett left a comment

Choose a reason for hiding this comment

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

I feel like HDL examples should be examples of clean synthesizable reusable code, which is where most of my heartache lies.

I don't know much about the base classes included in cocotb, but it seems like it's forcing your into a coding style that isn't very reusable. Monitors and Drivers should be able to be written to be reusable. Ideally, you should be able to write the entire testbench in such a way that the driving and analysis are segregated, so that you can reuse the analysis side to test the module (the DUT in this unit-level test) while it is driven by an external system. Such a refactor would be pretty extensive, so this is more of a comment.

examples/matrix_multiplier/hdl/matrix_multiplier.vhdl Outdated Show resolved Hide resolved
examples/matrix_multiplier/hdl/matrix_multiplier.v Outdated Show resolved Hide resolved
examples/matrix_multiplier/hdl/matrix_multiplier.v Outdated Show resolved Hide resolved
examples/matrix_multiplier/hdl/matrix_multiplier.v Outdated Show resolved Hide resolved
examples/matrix_multiplier/tests/test_matrix_multiplier.py Outdated Show resolved Hide resolved
@marlonjames
Copy link
Contributor Author

Thoughts on synchronous vs asynchronous resets?

@ktbarrett
Copy link
Member

ktbarrett commented Mar 17, 2020 •
edited

@garmin-mjames Synchronous IMO, but it doesn't really matter, we aren't here to dictate one approach over the other.

@cmarqu
Copy link
Contributor

cmarqu commented Mar 17, 2020

Asynchronous assert, synchronous deassert, active-low is ASIC style. But I agree with Kaleb, it doesn't really matter here.

@eric-wieser eric-wieser added this to the 1.4 milestone Mar 19, 2020
This was referenced Mar 19, 2020
Merged
Closed
@marlonjames marlonjames mentioned this pull request Mar 20, 2020
Closed
@marlonjames
Copy link
Contributor Author

Needs #1507 first.

@eric-wieser eric-wieser added the status:blocked further progress is blocked by a dependency, e.g. other code which must be commited first. label Mar 22, 2020
@marlonjames marlonjames mentioned this pull request Mar 23, 2020
Merged
@eric-wieser eric-wieser removed the status:blocked further progress is blocked by a dependency, e.g. other code which must be commited first. label Mar 30, 2020
@eric-wieser eric-wieser added the status:needs-rebase needs a git rebase label Mar 31, 2020
@marlonjames marlonjames removed the status:needs-rebase needs a git rebase label Jul 15, 2020
ktbarrett
ktbarrett reviewed Jul 20, 2020
View reviewed changes
Copy link
Member

@ktbarrett ktbarrett left a comment

Choose a reason for hiding this comment

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

Just tested with Questa 10.7c. All seems good, except that I had to add SIM_ARGS="-t ps" to get it to run. Questa defaults time precision to 1ns, it would be nice if regressions ran without any additional args.

examples/matrix_multiplier/tests/test_matrix_multiplier.py Outdated Show resolved Hide resolved
@marlonjames
Copy link
Contributor Author

@elgorwi Could you test the changes with Verilator?

@ktbarrett
Copy link
Member

ktbarrett commented Jul 21, 2020 •
edited

@garmin-mjames It fails. Generics are not being passed properly.

Verilator support -pvalue+name=value syntax like VCS, or -Gname=value. I tested with the pvalue syntax. I can't tell if it's hung or really slow. I'll come back in a half an hour and check.

@marlonjames
Copy link
Contributor Author

I couldn't tell from the Verilator documentation whether I need the top-level module name in the generic parameters. The examples in the docs don't have it, so I didn't put it in.

@ktbarrett
Copy link
Member

ktbarrett commented Jul 21, 2020 •
edited

Nvm me. I was a little behind, before you added support to the makefile for Verilator. It runs in Verilator. We should work on adding Verilator to the CI.

@marlonjames
Copy link
Contributor Author

Pinging @imphil.

@marlonjames marlonjames mentioned this pull request Aug 6, 2020
Open
@marlonjames marlonjames added the status:needs-review this PR needs at least one review label Aug 20, 2020
Add matrix_multiplier to examples.
b9d95e1 
Add matrix multiplication example and test. The test is heavily
weighted towards use of the BinaryValue class. The random seed is set
to a default value to provide a deterministic test for performance
regression.
@imphil imphil closed this Sep 9, 2020
@imphil imphil reopened this Sep 9, 2020
@imphil
Copy link
Member

imphil commented Sep 9, 2020

Close/reopen to get a fresh private CI run (which was failing before). If that one passes LGTM. Thanks a lot @garmin-mjames for your persistence on this one, it looks like a really nice example now -- and sorry for being so slow to actually getting back to you. You've at least earned a drink of your choice should we meet at some time 😄

@imphil imphil merged commit c77d1ca into cocotb:master Sep 9, 2020
@ktbarrett ktbarrett mentioned this pull request Oct 17, 2020
Merged
@marlonjames marlonjames deleted the matrix_mult branch November 26, 2020 04:20
@imphil imphil mentioned this pull request Jul 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmarqu cmarqu cmarqu left review comments

@elgorwi elgorwi elgorwi left review comments

@ktbarrett ktbarrett ktbarrett approved these changes

@imphil imphil imphil approved these changes

@eric-wieser eric-wieser Awaiting requested review from eric-wieser

Assignees
No one assigned
Labels
category:performance performance topics status:needs-review this PR needs at least one review
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@marlonjames @eric-wieser @ktbarrett @cmarqu @imphil @gretzteam @elgorwi