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

Add $macc_v2 #4818

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Add $macc_v2 #4818

wants to merge 11 commits into from

Conversation

povik
Copy link
Member

@povik povik commented Dec 13, 2024

What are the reasons/motivation for this change?

$macc is needlessly complex: the CONFIG parameter uses a convoluted encoding, and it has a redundant B port for single-bit summands.

With $macc_v2 the goal is to make it simpler, so much so that it's not unreasonable to write techmap rules matching on this cell in plain Verilog (as opposed to just deferring to maccmap).

Explain how this is achieved.

Changes done in v2 (edited):

  • replace "ports" with "products" and "addends" (collectively called "terms"); this removes nomenclature confusion with RTLIL-level ports
  • use simple encoding for parameters (akin to $mem_v2, which encodes a dynamic number of read/write ports in a similar manner)
  • remove the original B port semantics, instead use A/B ports to hold the two operands for each product, and introduce a C port for addends

Based off #4817, which can go in first to make this one easier to review.

TODO:

  • decide on the final shape of $macc_v2
  • add satgen support
  • add consteval support
  • fix techmap.v
  • test all basic scenarios
  • add simlib model
  • find the "adding a new cell" checklist
  • make booth work
  • Add to kernel/celltypes.h (incl. eval() handling for non-mem cells)
  • Add to InternalCellChecker::check() in kernel/rtlil.cc
  • Add to techlibs/common/simlib.v
  • Add to techlibs/common/techmap.v
  • Add to docs/source/CHAPTER_CellLib.rst (or just add a fixme to the bottom)
  • Maybe add support to the Verilog backend for dumping such cells as expression

@povik
Copy link
Member Author

povik commented Dec 13, 2024

to write techmap rules matching on this cell in plain Verilog (as opposed to just deferring to maccmap).

This use case is better served with having an extra port of plain summands, and leave A and B ports for products

So that you have

  Y = A_1*B_1 + A_2*B_2 + ... + A_n*B_n + C_1 + ... + C_m

where each A_j, B_j, C_j are an extracted bit range from A, B, C input ports respectively

Cc @phsauter also @widlarizer @whitequark (we touched on $macc semantics in #4316)

@povik
Copy link
Member Author

povik commented Jan 10, 2025

Rebased

techlibs/common/simlib.v Outdated Show resolved Hide resolved
techlibs/common/simlib.v Outdated Show resolved Hide resolved
@povik povik marked this pull request as ready for review January 24, 2025 12:23
@KrystalDelusion
Copy link
Member

This reminded me that cell help construction needs documenting. Still WIP but here's a link to the docs preview build. While not necessary, it would certainly make me happy for new cells being added to use the v2 style and give things titles, since at some point I would like to update everything in simlib.v to use it.

@povik
Copy link
Member Author

povik commented Jan 25, 2025

Sure, let me review that and update

@widlarizer
Copy link
Collaborator

What's the value in retaining plain summands (C port)? Is that for matching some kind of techmappable primitive?

@povik
Copy link
Member Author

povik commented Jan 27, 2025

What's the value in retaining plain summands (C port)?

As opposed to connecting the summands to the A/B ports, or not fusing the addition into the $macc cell at all?

@widlarizer
Copy link
Collaborator

Initially I was thinking as opposed to not holding those inside $macc_v2, but the former seems a legitimate solution as well

@povik
Copy link
Member Author

povik commented Jan 27, 2025

as opposed to not holding those inside $macc_v2

It can be more efficient to combine the operations as they can share the reduction tree.

but the former seems a legitimate solution as well

See the comment above, it's meant to make writing "techmap" rules less awkward

@widlarizer
Copy link
Collaborator

So, what do we want to techmap them to? For example, the existence of the redundant B port in $macc was also motivated by having reasonable techmapping rules to something, but this PR removes it. It's hard for me to see what features to keep when I don't understand what they're actually for

@povik
Copy link
Member Author

povik commented Jan 27, 2025

E.g. the ELAU implementation of a multiply-add: https://github.com/pulp-platform/ELAU/blob/master/src/AddMulSgn.sv

For example, the existence of the redundant B port in $macc was also motivated by having reasonable techmapping rules to something, but this PR removes it

I don't know of anyone writing Verilog rules matching on the $macc cell, but even if they did, they should be able to pick out the single-bit summands on the C port easily. The new cell is (1) equivalent in power to $macc (2) using simpler encoding; so it should be easier to work with the new cell for any use case

@widlarizer
Copy link
Collaborator

Ah, so all $macc use cases should be retained with $macc_v2, sounds good. Let's consider proving that with a test as an acceptance criterium for this PR

@povik
Copy link
Member Author

povik commented Jan 27, 2025

Ah, so all $macc use cases should be retained with $macc_v2, sounds good. Let's consider proving that with a test as an acceptance criterium for this PR

That should be already the case. With this PR Yosys stops emitting any $macc by its own, and then there are tests included with this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants