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

Port ContentAddressableMemory from #395 #573

Merged
merged 35 commits into from
May 14, 2024
Merged

Port ContentAddressableMemory from #395 #573

merged 35 commits into from
May 14, 2024

Conversation

lekcyjna123
Copy link
Contributor

@lekcyjna123 lekcyjna123 commented Jan 29, 2024

Yet another PR to port something from #395. This time I ported CAM, which was requested by @makz00 for the LSU implementation. To port CAM few other things were needed:

  • MultiPriorityEncoder
  • generate_based_on_layout
  • SimpleLayout

Resolves: #560
Resolves: #559


EDIT 03.05.2024:

I have uploaded a new set of commits with the refactored functionalities, changes:

  • Added CAM
  • Added MultiPriorityEncoder
  • Added support for hypothesis
  • Bump flake8 and black to remove false positives from 3.12
  • Fix few formatting issues because of flake8 update

@lekcyjna123 lekcyjna123 added the enhancement New feature or request label Jan 29, 2024
@lekcyjna123 lekcyjna123 requested a review from makz00 January 29, 2024 08:14
transactron/utils/amaranth_ext/elaboratables.py Outdated Show resolved Hide resolved
transactron/utils/amaranth_ext/elaboratables.py Outdated Show resolved Hide resolved
test/common/input_generation.py Outdated Show resolved Hide resolved
Comment on lines 181 to 182
m.submodules.encoder_addr = encoder_addr = MultiPriorityEncoder(self.entries_number, 1)
m.submodules.encoder_valids = encoder_valids = MultiPriorityEncoder(self.entries_number, 1)
Copy link
Member

Choose a reason for hiding this comment

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

These have a single output, so they are just standard priority encoders, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But PriorityEncoders are going to be removed from amaranth.lib and MultiPriorityEncoder is a more generic solution which will be useful in future.

transactron/utils/amaranth_ext/elaboratables.py Outdated Show resolved Hide resolved
class ContentAddressableMemory(Elaboratable):
"""Content addresable memory

This module implements a transactorn interface for the content addressable memory.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This module implements a transactorn interface for the content addressable memory.
This module implements a Transactron interface for a content-addressable memory.

Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to call it an interface, when there is no internal module which has a traditional interface?

Shouldn't you write a few words to explain what a content addressable memory is? Also, this is a very specialized kind of content-addressable memory - no non-destructive reads, no writes.

Copy link
Member

Choose a reason for hiding this comment

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

And also, no possibility of adding an eviction mechanism, for CAMs used as a cache.

m = TModule()

address_array = Array([Record(self.address_layout) for _ in range(self.entries_number)])
data_array = Array([Record(self.data_layout) for _ in range(self.entries_number)])
Copy link
Member

Choose a reason for hiding this comment

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

The data could be stored in a memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After change of interface I decided to not use the Memory because it would need 3 ports.

transactron/lib/storage.py Outdated Show resolved Hide resolved
@tilk
Copy link
Member

tilk commented Mar 14, 2024

This could be nice to have in Transactron (I see some use cases for it), but the work has stopped. :(

@lekcyjna123 lekcyjna123 marked this pull request as draft March 17, 2024 15:10
@lekcyjna123
Copy link
Contributor Author

I was able to run first test using hypothesis sadly this library is very fragile to the order of drawing arguments, so we have to provide deterministic order between different runs of tests, what as we all know is very hard in pysim. I have to think what this problem should be fixed.

@lekcyjna123
Copy link
Contributor Author

I have prepared different solution which should be less prone to the order of processes because all data are generated before test execution (so only result has to be deterministic). Normal test execution is fast (no observable difference to the solution with randoms), but the case reduction can take a lot of time (45s).

Below are two example messages with found failing examples. They are saved locally in .hypothesis directory, so each next run is fast.

obraz
obraz

@lekcyjna123 lekcyjna123 marked this pull request as ready for review May 3, 2024 15:02
@lekcyjna123 lekcyjna123 requested review from tilk and xThaid May 3, 2024 15:03
@@ -237,3 +238,77 @@ def elaborate(self, platform):
m.d.sync += self.valid.eq(self.requests.any())

return m


class MultiPriorityEncoder(Elaboratable):
Copy link

Choose a reason for hiding this comment

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

I was thinking that since the priority encoder is just a combinational logic, it would be nice if we could use it like:

idx, valid = prio_encoder(m, one_hot_signal)

instead of the boilerplate code

m.submodules.prio_encoder = prio_encoder = PriorityEncoder(cnt)
m.d.av_comb += prio_encoder.i.eq(one_hot_singal)
idx = prio_encoder.o
valid = ~prio.encoder.n

Copy link

@xThaid xThaid May 5, 2024

Choose a reason for hiding this comment

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

And another thing. It would be great if we could specify the bit order of the priority encoder. In theory by having one type of the priority encoder, we could get the other type by reversing the input and subtracting the outputs, but that requires a few adders.

I know that this is pretty much a feature request, so feel free to ignore it - it can be done later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added create_priority_encoder to add the automatic connections. Please see what you think about it.

Regarding to the different bit ordering I don't see a reason why we should add it, because it can be handled without subtraction. You pass the reversed array and you use indexes returned by encoder in this reversed array.

Copy link
Member

Choose a reason for hiding this comment

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

I have added create_priority_encoder to add the automatic connections.

First, that's a mouthful. Second, a shorthand for creating outputs_count=1 encoders would be nice, as the return type of the function complicates deconstruction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the name to the create additionally I have added create_simple as a special case for outputs_count=1.

def fixture_initialize_testing_env(self, request):
self._transactron_hypothesis_iter_counter = 0
self._transactron_base_output_file_name = ".".join(request.node.nodeid.split("/"))
self._transactron_current_output_file_name = self._transactron_base_output_file_name
Copy link
Member

Choose a reason for hiding this comment

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

The logic behind _transactron_current_output_file_name deserves a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@lekcyjna123 lekcyjna123 mentioned this pull request May 12, 2024
1 task
@tilk tilk requested a review from xThaid May 13, 2024 12:27
"""Content addresable memory

This module implements a content-addressable memory (in short CAM) with Transactron interface.
CAM is a type of memory where instead of predefined indexes there are used values feed in runtime
Copy link

Choose a reason for hiding this comment

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

typo: 'feed' -> 'fed'


This module implements a content-addressable memory (in short CAM) with Transactron interface.
CAM is a type of memory where instead of predefined indexes there are used values feed in runtime
as keys (smimlar as in python dictionary). To insert new entry a pair `(key, value)` has to be
Copy link

Choose a reason for hiding this comment

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

typo: 'smimlar'

@tilk tilk merged commit edf58b6 into master May 14, 2024
13 checks passed
@tilk tilk deleted the lekcyjna/port-cam branch May 14, 2024 10:37
github-actions bot pushed a commit that referenced this pull request May 14, 2024
@xThaid xThaid mentioned this pull request Jun 26, 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.

Port ExtendedEncoder from #395 Port CAM from #395
3 participants