[Packer] Pack Pattern Get Sink #2991
Conversation
github-actions
bot
added
VPR
There was a problem hiding this comment.
Hi @amin1377 , great changes! I left some minor comments, but other than that it looks good to me.
| @@ -1076,23 +1071,50 @@ static bool try_expand_molecule(t_pack_molecule& molecule, | |||
| /** | |||
| * Find the atom block in the netlist driven by this pin of the input atom block | |||
| * If doesn't exist return AtomBlockId::INVALID() | |||
| * Limitation: The block should be driving only one sink block | |||
There was a problem hiding this comment.
Isn't this limitation still there? Based on your code changes, it appears as though you have just handled the special case where this is a chain pattern.
Is it possible for these changes to be generalized? If its complicated I would leave as a TODO at least so people are aware of this limitation.
There was a problem hiding this comment.
I gave some thought to making it more general, but the only viable way I can think of to prevent the issue I mentioned, where both the registered and unregistered outputs of a LUT are used, is to allow breaking a molecule during packing. As you definitely know (!), that’s not a trivial task.
I’ve added the following TODO comment:
TODO: Limitation — For pack patterns other than chains,
the block should be driven by only one block
vpr/src/pack/prepack.cpp
Outdated
| if (is_chain_pattern) { | ||
| // If the pattern is a chain, allow nets with multiple sinks. | ||
| // This enables forming chains where the COUT is connected both to | ||
| // the next element in the chain and to the block's output pin. |
There was a problem hiding this comment.
Is this always valid? Is there ever a case where we could get a packing failure as you detail below?
Is it somehow possible to check for this?
There was a problem hiding this comment.
Here, I’m making the assumption that if a pin in the pack pattern has a fan-out greater than one, the architecture explicitly allows it. If you have a case where, for example, two adders are connected together and the carry connection between them is also connected to another block, but the architecture does not support this behavior, that would result in a problem.
However, without this update, even if the architecture does support such a mechanism, the design would still fail to implement correctly due to the current restriction.
vpr/src/pack/prepack.cpp
Outdated
| // if the net fanout is 1. To clarify, consider a case where the output of a LUT | ||
| // is connected to both a register and an unregistered output that feeds another block. | ||
| // If the intra-cluster architecture doesn't support having both registered and | ||
| // unregistered outputs simultaneously, this could lead to a packing failure. |
There was a problem hiding this comment.
Same comment as above. Is it possible to check the architecture and see if we can ignore this assumption. I am wondering if this can be made more general. If it is complicated, perhaps we can raise this as an issue to investigate further later.
Prepacking is super powerful, and the more intelligent we can make it the better in my opinion.
There was a problem hiding this comment.
Good point! Analyzing the architecture file to determine whether to consider multi-fanout nets in pack patterns is a good idea. I’ve created Issue #2996 to track this.
vpr/src/pack/prepack.cpp
Outdated
|
|
||
| if (port_id) { | ||
| auto net_id = atom_nlist.port_net(port_id, pin_number); | ||
| if (to_port_id) { |
There was a problem hiding this comment.
I recognize that this is not your code here, but I think the code can be made more readible if we reduce the nesting level of these if statements. A lot of the code in this file can be made more readible; so it would be nice to chip away at it!
There was a problem hiding this comment.
It’s always a good idea to leave the code a little better than you found it :) I’ll clean up the nesting here.
|
This PR is currently failing Nightly Tests...DO NOT MERGE! |
|
@AlexandreSinger The PR has now passed Nightly Test 1 (tested on Wintermute). Feel free to merge it if you don’t have any further comments. |
In the packer, molecules are formed first, followed by clusters. Molecules are created based on pack patterns defined in the architecture file. To form them, all possible molecule patterns are first extracted from the architecture and sorted in decreasing order of the number of blocks, so that larger pack patterns are attempted first.
Next, the atom netlist blocks are traversed to determine whether a pack pattern can be formed. When analyzing a candidate block, we check which blocks are connected to the pins involved in the pack pattern to decide if they can be included in the molecule. However, if the fanout of a net connected to a pack pattern pin is greater than one, the connected block is skipped.
This restriction exists to prevent issues in cases where a pack pattern includes a LUT and a register, and both the registered and unregistered outputs of the LUT are used. In architectures where only one of these outputs (registered or unregistered) can be routed to the LE output, forming such a molecule would result in a packing failure.
However, this restriction causes problems for patterns like carry chains. For example, in architectures where the COUT of a hard adder is both connected to the CIN of the next adder in the chain and to a separate block, the molecule is not formed. The packer sees that the COUT pin has multiple sinks and skips it.
To resolve this, this PR adds a special case for chain patterns: when analyzing a chain pattern, we no longer discard blocks connected to nets with fanout greater than one.