Skip to content

[Packer] Pack Pattern Get Sink #2991

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Apr 24, 2025
Merged

[Packer] Pack Pattern Get Sink #2991

Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6e86533
[vpr][pack] update prepack get_sink
amin1377 Apr 21, 2025
3497838
[vpr][pack] update prepack get_driving_block
amin1377 Apr 21, 2025
0008292
[pack][prepack] fix the bug with finding sink pin id
amin1377 Apr 22, 2025
a777170
[vpr][pack] skip if net is not valid
amin1377 Apr 22, 2025
f4c6ee8
[vpr][pack] consider nets with fan-out of more than one only if pack …
amin1377 Apr 22, 2025
7a4ff26
[vpr][pack] add comment
amin1377 Apr 22, 2025
17afb8c
make format
amin1377 Apr 22, 2025
6fa2422
[vpr][pack] remove nested ifs and add comments
amin1377 Apr 22, 2025
b4587bc
[vpr][pack] skip the blocck if it is connected to ff and another block
amin1377 Apr 23, 2025
48ba9b0
[vpr][pack] don't separate chain pattern w/ non-chain
amin1377 Apr 23, 2025
00d6d94
[vpr][pack] update comments
amin1377 Apr 23, 2025
d7549ca
[vpr][pack] remove is_chan_patter parameter
amin1377 Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 57 additions & 36 deletions vpr/src/pack/prepack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,12 @@ static void init_molecule_chain_info(const AtomBlockId blk_id,
const AtomNetlist& atom_nlist);

static AtomBlockId get_sink_block(const AtomBlockId block_id,
const t_model_ports* model_port,
const BitIndex pin_number,
const AtomNetlist& atom_nlist);
const t_pack_pattern_connections& connections,
const AtomNetlist& atom_nlist,
bool is_chain_pattern);

static AtomBlockId get_driving_block(const AtomBlockId block_id,
const t_model_ports* model_port,
const BitIndex pin_number,
const t_pack_pattern_connections& connections,
const AtomNetlist& atom_nlist);

static void print_chain_starting_points(t_pack_patterns* chain_pattern);
Expand Down Expand Up @@ -1047,17 +1046,13 @@ static bool try_expand_molecule(t_pack_molecule& molecule,
// this block is the driver of this connection
if (block_connection->from_block == pattern_block) {
// find the block this connection is driving and add it to the queue
auto port_model = block_connection->from_pin->port->model_port;
auto ipin = block_connection->from_pin->pin_number;
auto sink_blk_id = get_sink_block(block_id, port_model, ipin, atom_nlist);
auto sink_blk_id = get_sink_block(block_id, *block_connection, atom_nlist, molecule.is_chain());
// add this sink block id with its corresponding pattern block to the queue
pattern_block_queue.push(std::make_pair(block_connection->to_block, sink_blk_id));
// this block is being driven by this connection
} else if (block_connection->to_block == pattern_block) {
// find the block that is driving this connection and it to the queue
auto port_model = block_connection->to_pin->port->model_port;
auto ipin = block_connection->to_pin->pin_number;
auto driver_blk_id = get_driving_block(block_id, port_model, ipin, atom_nlist);
auto driver_blk_id = get_driving_block(block_id, *block_connection, atom_nlist);
// add this driver block id with its corresponding pattern block to the queue
pattern_block_queue.push(std::make_pair(block_connection->from_block, driver_blk_id));
}
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* block_id : id of the atom block that is driving the net connected to the sink block
* model_port : the model of the port driving the net
* pin_number : the pin_number of the pin driving the net (pin index within the port)
* connections : pack pattern connections from the given block
*/
static AtomBlockId get_sink_block(const AtomBlockId block_id,
const t_model_ports* model_port,
const BitIndex pin_number,
const AtomNetlist& atom_nlist) {
auto port_id = atom_nlist.find_atom_port(block_id, model_port);

if (port_id) {
auto net_id = atom_nlist.port_net(port_id, pin_number);
if (net_id && atom_nlist.net_sinks(net_id).size() == 1) { /* Single fanout assumption */
auto net_sinks = atom_nlist.net_sinks(net_id);
auto sink_pin_id = *(net_sinks.begin());
return atom_nlist.pin_block(sink_pin_id);
const t_pack_pattern_connections& connections,
const AtomNetlist& atom_nlist,
bool is_chain_pattern) {
const t_model_ports* from_port_model = connections.from_pin->port->model_port;
const int from_pin_number = connections.from_pin->pin_number;
auto from_port_id = atom_nlist.find_atom_port(block_id, from_port_model);

const t_model_ports* to_port_model = connections.to_pin->port->model_port;
const int to_pin_number = connections.to_pin->pin_number;
const auto& to_pb_type = connections.to_block->pb_type;

if (from_port_id) {
auto net_id = atom_nlist.port_net(from_port_id, from_pin_number);
if (net_id.is_valid()) {
const auto& net_sinks = atom_nlist.net_sinks(net_id);
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (const auto& sink_pin_id : net_sinks) {
auto sink_block_id = atom_nlist.pin_block(sink_pin_id);
if (primitive_type_feasible(sink_block_id, to_pb_type)) {
auto to_port_id = atom_nlist.find_atom_port(sink_block_id, to_port_model);
auto to_pin_id = atom_nlist.find_pin(to_port_id, BitIndex(to_pin_number));
if (to_pin_id == sink_pin_id) {
return sink_block_id;
}
}
}
} else {
// For non-chain patterns, we conservatively only consider the sink block
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (net_sinks.size() == 1) {
auto sink_pin_id = *(net_sinks.begin());
return atom_nlist.pin_block(sink_pin_id);
}
}
}
}

Expand All @@ -1104,33 +1126,32 @@ static AtomBlockId get_sink_block(const AtomBlockId block_id,
* If doesn't exist return AtomBlockId::INVALID()
* Limitation: This driving block should be driving only the input block
* block_id : id of the atom block that is connected to a net driven by the driving block
* model_port : the model of the port driven by the net
* pin_number : the pin_number of the pin driven by the net (pin index within the port)
* connections : pack pattern connections from the given block
*/
static AtomBlockId get_driving_block(const AtomBlockId block_id,
const t_model_ports* model_port,
const BitIndex pin_number,
const t_pack_pattern_connections& connections,
const AtomNetlist& atom_nlist) {
auto port_id = atom_nlist.find_atom_port(block_id, model_port);
auto to_port_model = connections.to_pin->port->model_port;
auto to_pin_number = connections.to_pin->pin_number;
auto to_port_id = atom_nlist.find_atom_port(block_id, to_port_model);

if (port_id) {
auto net_id = atom_nlist.port_net(port_id, pin_number);
if (to_port_id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s always a good idea to leave the code a little better than you found it :) I’ll clean up the nesting here.

auto net_id = atom_nlist.port_net(to_port_id, to_pin_number);
if (net_id && atom_nlist.net_sinks(net_id).size() == 1) { /* Single fanout assumption */

auto driver_blk_id = atom_nlist.net_driver_block(net_id);

if (model_port->is_clock) {
if (to_port_model->is_clock) {
auto driver_blk_type = atom_nlist.block_type(driver_blk_id);

// TODO: support multi-clock primitives.
// If the driver block is a .input block, this assertion should not
// be triggered as the sink block might have only one input pin, which
// would be a clock pin in case the sink block primitive is a clock generator,
// resulting in a pin_number == 0.
VTR_ASSERT(pin_number == 1 || (pin_number == 0 && driver_blk_type == AtomBlockType::INPAD));
VTR_ASSERT(to_pin_number == 1 || (to_pin_number == 0 && driver_blk_type == AtomBlockType::INPAD));
}

return atom_nlist.net_driver_block(net_id);
return driver_blk_id;
}
}

Expand Down
Loading