Partitioner Feature Additions: Mapping generation, net-span partition report, LUTs/partition report, verbose hypergraph reports, LUTCount CLI, mapping constraint partition input #1343
Conversation
…partition report, human readable hypergraph reports Signed-off-by: Perry Newlin <perry.newlin@amd.com>
pnewlin-amd
changed the title
clavin-xlnx
left a comment
clavin-xlnx
left a comment
There was a problem hiding this comment.
First round of feedback, please reach out with questions. I didn't make comments on every occurrence, but hopefully you'll be able to apply the patterns across all the changes.
Thanks for the PR!
| /** | ||
| * Memory audit utility for reading netlists. | ||
| */ | ||
| public class ReadNetlist { |
There was a problem hiding this comment.
This class seems to duplicate functionality in the CodePerfTracker class. This class is employed when loading a DCP and, when set with the right options, can produce a similar report. When calling Design.readCheckpoint() there are overload options to provide a custom configured CodePerfTracker instance. Have a look to see if that would meet your needs.
I'm thinking that having something like a DesignAnalyzer that can provide an array of analysis options while simply loading a design would be useful, but I'm not sure this is the right PR for it.
| * - counts: aggregated counts over this node’s subtree (leafCounts + all children) | ||
| * lets us easily decide if wholly contained or not | ||
| */ | ||
| private static final class Node { |
There was a problem hiding this comment.
Node is an overloaded term. Perhaps we could refactor to something more unique such as HierNode, or something similar.
| /** | ||
| * CLI utility to count logic LUT usage from EDIF (.edf/.edif) or Vivado DCP (.dcp). | ||
| */ | ||
| public final class LUTCount { |
There was a problem hiding this comment.
I think the functionality of this class should be absorbed into a DesignAnalyzer class. See my comment on ReadNetlist.java.
| Files.createDirectories(parent); | ||
| } | ||
| Files.write(out, lines, StandardCharsets.UTF_8); | ||
| } catch (IOException e) { // more guards the better. |
There was a problem hiding this comment.
Not sure what you mean by this comment?
| * @param partitions A map of partition IDs to sets of instance names. | ||
| * @param instLutCountMap A map of instance LUT counts (unused but kept for API consistency). | ||
| */ | ||
| public static void write(Path outDir |
There was a problem hiding this comment.
This method is confusingly named as the Javadoc says it writes the mapping file, but there is a method just above it that is called writeMappingFile().
|
|
||
|
|
||
|
|
|
|
||
|
|
||
| // ============================================================================ | ||
| // ARGUMENT PARSING & INITIALIZATION |
There was a problem hiding this comment.
It seems like main() is broken up into various tasks, but they are all in the same method (its over 700 lines long). This should be broken up into separate methods.
| int backfillAttempts = 0; | ||
| int backfillSuccess = 0; | ||
| int backfillSkipped = 0; | ||
| java.util.Map<com.xilinx.rapidwright.edif.EDIFCell, java.lang.Integer> lutCountCache; |
There was a problem hiding this comment.
Please avoid fully qualified references and instead just import the class at the top of the file.
| @@ -0,0 +1,62 @@ | |||
| /* | |||
There was a problem hiding this comment.
Not sure if this functionality needs its own class. Perhaps refactor to make it a bit more generic and put it in StringTools?
| String partitionLabelB = pairKey.substring(separatorIndex + 1); | ||
| int pairCount = pairCounts.get(pairKey); | ||
|
|
||
| //format will be "FPGA_A--count--FPGA_B". |
There was a problem hiding this comment.
| //format will be "FPGA_A--count--FPGA_B". | |
| // format will be "FPGA_A--count--FPGA_B". |
2 participants
A series of QoL features and additions to the partitioner branch.
TOP/Counteris wholly contained in partition 1 (FPGA_B) then produce only one lineTOP/Counter FPGA_B