-
Notifications
You must be signed in to change notification settings - Fork 12
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
Allow specification of multiple pack patterns #75
base: main
Are you sure you want to change the base?
Conversation
Requires #76 for the CI to pass. |
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.
Please rebase and add a test.
98cea53
to
d35c699
Compare
@mkurc-ant - I think we can merge this after the doc update? |
@mithro Yes. Once docs update is merged I'll add docs to this one. |
d35c699
to
0ec9343
Compare
@mithro I've rebased and updated docs. |
tests/pack_pattern/README.rst
Outdated
@@ -0,0 +1,36 @@ | |||
Pack pattern annotation example |
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.
Your test examples makes sense. However, I think v2x should be automatically detecting the requirement for a pack-pattern here and then generate these pack pattern names automatically.
A pack pattern is needed anytime there is an "internal pathway" which isn't connected to a top level port. IE
[ ]->A->B->[ ]
- the internal signal A->B
requires a pack pattern to be generated. I'm pretty sure we should be able to detect these internal only pathways?
In your example we have;
[ ] --> LUT --> MUX ------> [ ]
\
\-> FF -> [ ]
There are two "internal pathways", they are;
LUT -> MUX
pathway.LUT -> MUX -> FF
pathway.
The names of the pack patterns can easily be generated as <name_of_lut>_<name of mux>
and <name_of_lut>_<name of mux>_<name of ff>
. (Or some other name.)
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.
The idea is sound. I'm trying to picture a scenario where it wouldn't be applicable.
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.
@mithro So I've done some thinking and I think that automatic pack-pattern inference won't work in general. I got some corner cases:
First of all V2X is not aware of the whole hierarchy of pb_types. It operates only on a given level at a time. This makes impossible to infer a pack pattern that would span multiple layers of the hierarchy. Such pack patterns will be required when modelling complex architectures like eg. IOB and I/OLOGIC in 7-series. Moreover V2X won't be able to generate a name for it as the name has to be unique throughout the hierarchy.
My second thought: What if there are multiple connections between cells? Specifying pack pattern on both of them would make it effective only when both connections are used. What if it is not the case?
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.
Can you post actual examples of these?
@mkurc-ant -- Where are we at with this pull request? Can you rebase? |
0ec9343
to
4eccdff
Compare
@mithro I've rebased and re-arranged pack pattern examples/tests. |
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.
The forking example @ https://python-symbiflow-v2x--75.org.readthedocs.build/en/75/examples/pack_pattern/forking/README.html shouldn't need (* pack *)
attribute right? We can auto-detect that case?
); | ||
|
||
// LUT | ||
(* PACK="lut_mux_ff" *) |
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 thought we agreed to auto-generate the names?
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.
@mithro I think that the pack feature should have two different ways of working:
- fully-automated: this would be as you suggested in Rework pack pattern annotation generation #89, and still needs to be implemented. This would be triggered with the
(* pack *)
attribute and the pack_pattern attribute will be inferred to be equal to the combined names of the cells the net traverses. - manual: this is already implemented and enhanced in this PR (which allows to manually define mutliple pack patterns for spefic connections). This would be allowed with the explicit
(* PACK=... *)
attribute, giving more control to the designer.
Having only a fully-automated way of generating the pack patterns might lead to adding pack patterns where they are not needed. For instance, if the (* pack *)
attribute is added to a wire in the top-level module, and the hierarchy spans for multiple levels, we would likely lose the possibility to control the generated-pack patterns.
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
4eccdff
to
00dff88
Compare
This PR enhances the
PACK
attribute support so that multiple pack patterns can be assigned to a wire.