Chain pack pattern assertion fix #1591
Conversation
…ggered incorrectly Signed-off-by: Maciej Kurc <[email protected]>
|
@mkurc-ant - Could you post some pictures of what you are describing? |
|
@mithro This is a schematic of the PB-CLB complex block from the attached In the |
| @@ -1548,6 +1548,7 @@ static void update_chain_root_pins(t_pack_patterns* chain_pattern, | |||
| for (const auto pin_ptr : chain_input_pins) { | |||
| std::vector<t_pb_graph_pin*> connected_primitive_pins; | |||
| get_all_connected_primitive_pins(pin_ptr, connected_primitive_pins); | |||
| VTR_ASSERT(connected_primitive_pins.size()); | |||
There was a problem hiding this comment.
Suggest a comment here on the assert. I think the assert is checking that some primitive pin in some mode connects to the cluster level chain input pin.
|
Thanks for the detailed description and the code looks fine. I suggest adding a comment on what the assert is checking though, as this PR shows this is a pretty subtle test. |
|
Merged this since CI is green, but I think a comment is still a good idea. |
|
@vaughnbetz I'm sorry that I haven't added the comment yet. I'm currently on a short holiday leave. I'll add it one I'm back |
|
No worries and have a good holiday! |
This PR moves an assertion related to chain pack-patterns elsewhere to allow for pb_type modes that break a chain.
Description
Whenever there is a chain pack-pattern that passes through a pb_type with a mode in which chain-related pins are unconnected VPR raises an assertion. This happens at the pre-packing stage (the circuit doesn't matter). I've attached an example arch.xml file to allow reproduction of the issue.
In the attached example
arch.xmlthere is a top-level pb_typePB-CLBthat has two modes:LUT4andLUT4_CARRY. The former hosts theLUT4BLIF model (a classic LUT4) while the latter hosts a LUT that also has carry input and output pins for adder implementationLUT4_CARRY.The assertion is throws specifically when the interconnect of the
LUT4mode includes the carry-chain pin connections to the underlying pb_type where they are actually unconnected. The assertion says:which is kind of misleading.
By digging into the code I've realized that this assertion is placed incorrectly as it is being triggered when no connected pin is found within a specific mode which does not necessary mean that the pin is not connected in other modes.
My proposal in this PR is to move the assertion right after the recursive check done in
get_all_connected_primitive_pinsto allow for such situations. This way the assertion will get triggered only if a carry pin is completely disconnected (in all modes).Related Issue
None
Motivation and Context
I'm using the V2X tool (https://github.com/SymbiFlow/python-symbiflow-v2x) to generate pb_type XMLs. The tool automatically generate interconnects for modes regardless whether all pins are connected inside a mode.
I found that the issue can be worked around by simply removing the unconnected pin connections from a mode interconnect. However, I'd like the architecture specification to be more flexible as having such unconnected pins is not an error as the carry chain isn't really broken. Its just requires a specific mode to be selected to complete it. This is the reason for this PR.
How Has This Been Tested?
I wrote a test arch.xml file that is attached below to demonstrate the issue. The upstream VPR (i.e. from master) fails with it while the one with the fix from this PR completes successfully.
arch.xml.tar.gz
The issue can be reproduced by running packing of any circuit with this architecture. The fail occurs at the prepacking stage and is not related to the circuit itself but rather to the arch.
Types of changes
Checklist:
I'm not sure yet how to include the test with the attached arch.xml into the VTR flow but I can do that if it is requested.