-
Notifications
You must be signed in to change notification settings - Fork 2
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
Wide FIFO #23
Wide FIFO #23
Conversation
@@ -0,0 +1,188 @@ | |||
from amaranth import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstrings will be welcome, so that user can check API on the docs webpage. It would be good to underline the difference between Amaranth provided shifts and rotates, and these ones. I haven't found at the start, that this are signal based shifts in contrast to constants in Amaranth lib.
def generic_shift_vec_right( | ||
data1: Sequence[ValueLike | ValueCastable], data2: Sequence[ValueLike | ValueCastable], offset: ValueLike | ||
) -> Sequence[Value | ValueCastable]: | ||
shape = shape_of(data1[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be here an assert for non-zero length of data1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
m.submodules[f"storage{i}"] = mem | ||
|
||
write_ports = [mem.write_port() for mem in storage] | ||
read_ports = [mem.read_port(domain="sync", transparent_for=[port]) for mem, port in zip(storage, write_ports)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the transparency needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so, same as in BasicFifo
. Explanation: if the FIFO is empty and a write
is performed in cycle 1, then if read
is called in cycle 2, it needs to return the value written in cycle 1, which is provided thanks to transparency. If you remove transparency, the test fails.
m.d.comb += next_read_row.eq(read_row) | ||
m.d.comb += next_read_col.eq(read_col) | ||
m.d.sync += read_row.eq(next_read_row) | ||
m.d.sync += read_col.eq(next_read_col) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High surprise factor. A comment in the code will be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought it was a popular design pattern in HDLs. Still, I will leave a comment.
test/utils/test_shifter.py
Outdated
(shift_vec_right, lambda mkc: [], lambda val, offset, mkc: val[offset:] + [mkc(0)] * offset), | ||
( | ||
shift_vec_left, | ||
lambda mkc: [("placeholder", mkc(0))], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be mkc(1)
? With 0
test is nearly the same as previous shift_vec_left
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix.
test/lib/test_fifo.py
Outdated
count = random.randint(1, self.write_width) | ||
data = [const_of(random.randrange(2**self.bits), self.shape) for _ in range(self.write_width)] | ||
await self.circ.write.call(sim, count=count, data=data) | ||
await sim.delay(2e-9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of 2e-9
is not very clear to me. Why that value and not 1e-9
? Shouldn't we use some variable with cycle length instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because 1e-9 is used in a different testbench process. This is used to enforce execution order without synchronization primitives, which sadly are not provided (?) by Amaranth.
This PR introduces a wide FIFO data structure, which allows writing and reading multiple entries in a cycle. The entries are left-aligned and the order is preserved.
TODO:
__all__
#22