More metrics for RF, RS and ROB #632
Conversation
test/transactron/test_metrics.py
Outdated
|
|
||
| time = 0 | ||
|
|
||
| def ticker(): |
There was a problem hiding this comment.
You can use yield Now() instead of creating separate process.
There was a problem hiding this comment.
The test is written similar to to other metrics tests. I'm not going to do the change here without changing all the others. This is best left for a refactoring PR.
| for _ in range(200): | ||
| if not free_slots: | ||
| yield | ||
| continue |
There was a problem hiding this comment.
This cause that we can have less than 200 iterations. Was this intentional?
There was a problem hiding this comment.
Not really. I guess changing that if to while should be enough.
test/transactron/test_metrics.py
Outdated
| self.assertEqual(min(latencies), (yield m._dut.histogram.min.value)) | ||
| self.assertEqual(max(latencies), (yield m._dut.histogram.max.value)) | ||
| self.assertEqual(sum(latencies), (yield m._dut.histogram.sum.value)) | ||
| self.assertEqual(len(latencies), (yield m._dut.histogram.count.value)) | ||
|
|
||
| for i in range(m._dut.histogram.bucket_count): | ||
| bucket_start = 0 if i == 0 else 2 ** (i - 1) | ||
| bucket_end = 1e10 if i == m._dut.histogram.bucket_count - 1 else 2**i | ||
|
|
||
| count = sum(1 for x in latencies if bucket_start <= x < bucket_end) | ||
| self.assertEqual(count, (yield m._dut.histogram.buckets[i].value)) |
There was a problem hiding this comment.
Could we do that common with FIFOLatencyMeasurer test?
There was a problem hiding this comment.
Possibly, maybe that's a refactor worth doing.
| @@ -41,10 +43,13 @@ def __init__(self, gen_params: GenParams, func_units: Iterable[tuple[FuncUnit, s | |||
| Functional units to be used by this module. | |||
| rs_entries: int | |||
| Number of entries in RS. | |||
| rs_number: int | |||
| The number of this RS block. Used for debugging. | |||
There was a problem hiding this comment.
If used for debugging, maybe there should be a default value? So that if anyone doesn't care about debug feature, then it doesn't have to pass that value?
There was a problem hiding this comment.
I thought about it, and I don't think this is a good thing. If rs_number is not set in a given CoreConfiguration then the metric will be hard to read, so the person doing the metrics will need to change the configuration.
Probably it would be better to auto-generate these numbers.
|
It seems like the test might still have a race. Those |
This PR adds the following metrics:
Conclusions from reading the metrics:
The PR also adds two modules:
AsyncMemoryBankwhich basically wraps an Amaranth memory with asynchronous reads,IndexedLatencyMeasurerwhich allows to measure latency for things that are not processed in FIFO fashion, but have unique indexes (like RF and RS entries).TODO: