docs: Document $macc #4316
docs: Document $macc #4316
Conversation
|
Contributes to #121 |
| u(num_bits) factor1_len; | ||
| u(num_bits) factor2_len; | ||
| }[num_ports]; | ||
| }; |
There was a problem hiding this comment.
@widlarizer Thank you for documenting this!
@ everyone-else The semantics for this is completely insane, right? I'm not alone in wanting to see someone widlarize it? Why does it have dependently typed bitfields? The biggest factor[12]_len it can express is u16. Why not just use u16 at all times, so that you do not need dependently typed bitfields?
There was a problem hiding this comment.
I consider it an extreme case of data oriented design, probably to the extent of premature optimization. The complexity makes it probably liable for bugs if, say, somebody wrote HLS using Yosys that tries to generate $macc outside of yosys itself. Special casing single bit arguments, special casing zero length second multiplication arguments instead of using constants - I'm interested what motivated this
There was a problem hiding this comment.
The biggest factor[12]_len it can express is u16. Why not just use u16 at all times, so that you do not need dependently typed bitfields?
It's a bit worse, the maximum is u15. We could restrict the definition of $macc going forward, we could say that they are only valid with 15 for num_bits.
It's more complex than it needs to be, but I am not convinced that alone justifies changing it right now. If we had other changes we would want to make in the cell's definition, then sure, let's go for $macc_v2 that's as simple as we can make it.
There was a problem hiding this comment.
I change my position. We can make this much simpler. We can express all of this through a sequence of products on signals, where those signals can be constant as need be. We can afford to make all of the operands the same width, though that may be going too far. I am in favor of defining macc_v2.
There was a problem hiding this comment.
Are we externally bound to keep macc unchanged? Replacing all uses of current macc with simpler macc seems easy
There was a problem hiding this comment.
Are we externally bound to keep macc unchanged?
I can't imagine anyone outside of YosysHQ is using this mess...
There was a problem hiding this comment.
I put "defining $macc_v2" as an agenda item for the next dev JF.
I'll bring this up at the docs JF, it might be worthwhile to dump the help output for all of the cells like we do with commands. |
|
I pushed a fixup commit to the branch on my fork but it's not showing up in this PR 🤨 |
| The ``$macc`` cell type represents a multiply and accumulate block, for summing any number of negated and unnegated signals and arithmetic products of pairs of signals. Cell port A concatenates pairs of signals to be multiplied together. When the second signal in a pair is zero length, a constant 1 is used instead as the second factor. Cell port B concatenates 1-bit-wide signals to also be summed, such as "carry in" in adders. | ||
|
|
||
| The cell's ``CONFIG`` parameter determines the layout of cell port ``A``. | ||
| In the terms used for this cell, there's mixed meanings for the term "port". To disambiguate: |
There was a problem hiding this comment.
Let's call the "multiplier ports" factors or some other term. Overloading the term "port", especially in a document explaining the cell library, will make it hard on the reader.
There was a problem hiding this comment.
Ah "multiplier port" seems to be a "factor pair" rather than a factor
There was a problem hiding this comment.
Overloading the term "port", especially in a document explaining the cell library, will make it hard on the reader.
I agree, but the problem is, the term is already overloaded in the internal naming of variables and fields. I was following that. I can refactor as followup or as part of this PR
There was a problem hiding this comment.
I can refactor as followup or as part of this PR
If you mean updating the code for a different naming of the internal variables, I would say there's no need. We can afford some naming mismatch of this kind. Especially if we may supersede $macc with $macc_v2 soon, which means we will touch all of that code anyway.
There was a problem hiding this comment.
I have removed the multiplier port concept from .rst and left it in simlibs since the "port" variables are actually used there
| .. code-block:: | ||
| :force: | ||
| Y = port0factor1 * port0factor2 + port1factor1 * port1factor2 + ... | ||
| * B[0] + B[1] + ... |
There was a problem hiding this comment.
I fixed it, even if +- is a confusing operator
widlarizer
force-pushed
the
emil/document-macc
branch
from
836cadc to
9510293
Compare
|
LGTM |
|
FYI I am going to look into automatically including the verilog and comments from the simlib for each cell type into the docs, which will probably deprecate cell_library.rst but we can deal with that later. |
I did my best to frontload the description with a bit that's free of CONFIG technicalities. This PR unfortunately duplicates information between cell library docs and simlib.v, where the first paragraph is pulled as the output of
help $macc