Relatively Placed Macro - #3717
Conversation
…g attraction groups
…rilog-to-routing into relative_placement
…rilog-to-routing into relative_placement
…rilog-to-routing into relative_placement
…rilog-to-routing into relative_placement
AlexandreSinger
left a comment
There was a problem hiding this comment.
I got up to and including greedy_clusterer.h
I had a lot of blocking comments on this PR. We can discuss offline if needed.
|
|
||
| ``site_path`` is the hierarchical path of the primitive the atom was placed on within its | ||
| cluster (``t_pb_graph_node::hierarchical_type_name()``), including the mode selected at each | ||
| level. |
There was a problem hiding this comment.
This is going to BALLOON the flat placement file size. We should make this optional in my opinion. Can we add a CLI argument so the user can request this?
There was a problem hiding this comment.
I agree that this is useful, but AP does use this file as an intermediate file (when used to start from specific stages).
There was a problem hiding this comment.
I added a new CLI parameter, --flat_place_verbose, which is off by default. We can also use it later if we want to include more information in the flat place file.
| "of the architecture.\n", | ||
| atom_ctx.netlist().block_name(atom).c_str(), | ||
| site_path.c_str()); | ||
| } |
There was a problem hiding this comment.
I do not like this. I think this should be a warning and nothing should be printed. Something like "ERROR" in the file is sufficient to me. Failing the entire flat place file write for optional metadata is very strange to me.
There was a problem hiding this comment.
Unless you can prove that its impossible to get spaces from the hierarchical_type_name method; but we would need a guarantee that it implies a bug.
| static_cast<size_t>(blk_id), | ||
| atom_pbgn->pb_type->name); | ||
| atom_pbgn->pb_type->name, | ||
| site_path.c_str()); |
There was a problem hiding this comment.
See my comment from the documentation. I think that the site_path should be optional.
| * | ||
| * @return The id of the newly added macro. | ||
| */ | ||
| UserRelativeMacroId add_macro(const UserRelativeMacro& macro); |
There was a problem hiding this comment.
I like this class a lot since it manages the existing macros in the design (similar to the netlist). Normally for these type of classes, we construct the objects being managed inside the class itself. So I would expect to see a create_macro class which takes the needed arguments. This instead passes in an object and copies it in. This seems a bit odd to me from an API perspective since for a time there will be two copies of the same object in the codebase.
Is there a way that we can have this class construct the macro itself?
I understand that there may be limitation due to the XML parsers; I understand if this is cleaner. If so, you should update the comment slightly to say that this class will now maintain the object. A better solution is to use move semantics for this function: this would imply that ownership is being transferred to this class. The vector of strings in the macro will also be expensive to copy; so using move semantics would also be much faster.
| /// @brief Reverse lookup: atom -> the hierarchical path of the primitive | ||
| /// site it is locked to. Only holds the locked atoms; a missing atom | ||
| /// is unlocked. | ||
| std::unordered_map<AtomBlockId, std::string> atom_to_site_path_; |
There was a problem hiding this comment.
This seems a bit expensive to store for what it is. These site paths can be very very long, so the strings are very large. It would be good to only keep a single copy of them in the datastructures.
Why not just use the atom_to_group variable and get the path from within the macro?
| legalization_cluster_id, | ||
| cluster_legalizer, | ||
| attraction_groups); | ||
| } |
There was a problem hiding this comment.
This is speicific to relative groups. I think its confusing to have here. I think we should have a different code path for relative groups.
| // matches the sites in the constraints file, so it is not worth keeping | ||
| // while another sequence is still untried. On the last attempt the cluster | ||
| // is kept regardless - the leftovers are then reported as a split group by | ||
| // the end-of-pass check. |
There was a problem hiding this comment.
I do not understand what this comment is saying. Why does the last attempt matter here?
| + "). A site path also fixes the block type and the mode at every level, so a " | ||
| "path recorded on another architecture or another mode never matches. " | ||
| "Regenerate the macro against this netlist and architecture.\n"; | ||
| } |
There was a problem hiding this comment.
This should be outlined to relative macro specific code.
| // equivalent LUT/crossbar inputs each net enters through greedily as | ||
| // molecules arrive, so a sequence that leaves two nets contending for one | ||
| // input fails where another routes. No variant can move an atom off its | ||
| // site, and verify_clustering re-checks that afterwards. |
There was a problem hiding this comment.
I think this is far too much for the packer to deal with. Its super strange to me that we are reversing this explicitly here.
Why not push this on the user? We are going to pack in constraint-file order. If the user put things in a bad order, we can tell them to reverse it. Its a bit arbitrary to me that reversing the order would all of a sudden fix this issue.
| // group-4 flip-flops are locked into FF slots of arithmetic-mode FLEs that | ||
| // the group's own 40-atom carry chain claims first. Molecules that are | ||
| // never admitted are reported as a split group by the end-of-pass check in | ||
| // pack.cpp. |
There was a problem hiding this comment.
I am not following what this paragraph is saying.
This PR adds support for user-defined relative placement macros (RPMs) in VPR constraint files. An RPM fixes the placement of groups of primitives relative to one another without assigning absolute locations, preserving the internal geometry of a hand-optimized block (a DSP with its control logic, a bit-sliced datapath, an IP block) while leaving the placer free to choose where the whole macro goes.
Usage
Add a
<relative_macro_list>section to the VPR constraints file passed with--read_vpr_constraints(alongside the existing<partition_list>):Each macro has one
<reference_group>(the anchor) and one or more<relative_group>elements at fixed(x, y, sub_tile)offsets from it.<add_atom>uses the same matching semantics as in a<partition>.The group/offset constraints control placement at cluster granularity. To also control placement inside a cluster,
<add_atom>optionally takes asite_pathattribute that pins the atom to a specific primitive site within its group's cluster. The site is given as the hierarchical path of the primitive in the cluster's pb hierarchy, e.g.:The packer places the atom on exactly that site (packing fails with an error if it can't); atoms without a
site_pathare placed on whatever site the packer picks. Together, offsets plussite_pathallow reproducing a known-good placement exactly, down to the primitive level.The atoms of each group are packed into a single cluster (groups never share a cluster), and the resulting clusters are placed as one rigid macro — like an architectural carry chain, but supporting heterogeneous block types. The final placement is validated against the constraints.
See
doc/src/vpr/relative_placement_constraints.rstfor the file-format reference, detailed semantics, and restrictions.Notes
Testing
Unit tests for the constraints data model, plus a new
vtr_reg_strongtask (strong_relative_placement) covering mixed-type macros, regex matching, molecule interactions, and end-to-end packing/placement. Also validated on Titan benchmarks with constraint sets derived from baseline placements: all macros placed at their exact offsets with no consistency violations.