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.

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

Hardware Metrics #580

Merged
merged 11 commits into from
Feb 12, 2024
Merged

Hardware Metrics #580

merged 11 commits into from
Feb 12, 2024

Conversation

xThaid
Copy link

@xThaid xThaid commented Feb 1, 2024

The main part of PR #572, which adds the core classes for hardware metrics along with some unit tests.

@xThaid xThaid added the enhancement New feature or request label Feb 1, 2024
Copy link
Contributor

@lekcyjna123 lekcyjna123 left a comment

Choose a reason for hiding this comment

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

Overall looks good, but I am not sure if it isn't overly complicated.

coreblocks/structs_common/hw_metrics.py Outdated Show resolved Hide resolved
coreblocks/structs_common/hw_metrics.py Outdated Show resolved Hide resolved
coreblocks/structs_common/hw_metrics.py Outdated Show resolved Hide resolved
coreblocks/structs_common/hw_metrics.py Outdated Show resolved Hide resolved
Comment on lines 245 to 246
Additionally, the histogram tracks the number of observations, the sum
of observed values, and the minimum and maximum values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are they needed? E.g. number of observations we can calculate by summing up all elements from buckets.

Copy link
Author

Choose a reason for hiding this comment

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

we could, but it is more convenient to just look at one number. And it doesn't cost us much to have it

coreblocks/structs_common/hw_metrics.py Outdated Show resolved Hide resolved
from ..common import *


class CounterInMethodCircuit(Elaboratable):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you can not use SimpleTestCircuit?

Copy link
Author

Choose a reason for hiding this comment

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

Because metric methods are just wrappers and SimpleTestCircuit won't handle it.

Comment on lines 84 to 85
val = yield m._dut.counter.count.value
self.assertEqual(val, called_cnt)
Copy link
Contributor

Choose a reason for hiding this comment

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

More logical in my opinion it would be to have that code before if, because you compare counter to the previous one.

Copy link
Author

Choose a reason for hiding this comment

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

It won't work. It takes one cycle to update the value of the underlying counter, so if the method is called in cycle n, the counter will be increased in cycle n+1. Thus in fact I need to compare it with the previous value of the register.

And I don't want to add another yield after the method call, because I want to test how the counter works when it is possibly called every cycle (without stalls).

I added a clarifying comment to the code.


return m

def incr(self, m: TModule):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you decided for such interface instead of allowing to the direct calls to the _incr?

Copy link
Member

Choose a reason for hiding this comment

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

The first statement of this function is the answer. When metrics are disabled, the calls are not made.

test/structs_common/test_hw_metrics.py Outdated Show resolved Hide resolved
@xThaid xThaid force-pushed the hardware_metric_base branch from d370ec3 to 1116f65 Compare February 11, 2024 19:32
@lekcyjna123
Copy link
Contributor

I have created #585 to fix pipeline problem.

@tilk tilk merged commit 263720c into kuznia-rdzeni:master Feb 12, 2024
8 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 12, 2024
tilk pushed a commit to kuznia-rdzeni/transactron that referenced this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants