[Buffers] fix non-CFDFC paths having non-zero occupancy - #979
Conversation
chore: add constraints 10 and 15
| auto latOrFail = | ||
| timingDB.getLatency(node.op, SignalType::DATA, targetPeriod); | ||
| if (succeeded(latOrFail) && *latOrFail > 0.0) | ||
| constPart += *latOrFail; |
There was a problem hiding this comment.
Where is II in the original constraint?
|
Another thing that came across: could you generate a |
|
Not related to the PR but we should remember to check if it helps |
|
Hi, turns out I thought adding the filtering was going to be enough so I kept the style where I formulated the constraint for all channels in a CFDFC. This is bad, since we emit one global equality per path pair, instead of one per CFDFC and a path shared by two CFDFCs with different IIs needs a separate constraint under each II, and the code was disregarding that. Also good catch on missing the II division, I think it was indirectly caused by the above. Now that we do per-CFDFC we can use the entries in Also tried the setting of |
| /// to ensure sufficient buffering in faster loops. | ||
| /// (Paper: Section 5, Equation 15): Making the required occupancy the | ||
| /// maximum of all CFDFCs' II. | ||
| /// Determine which channels belong to at least one CFDFC. |
There was a problem hiding this comment.
Could you add a remark saying why we care about this? (i guess this is for setting the occupancy to be 1 for every channel that is not part of a cfdfc, but has latency > 0?
| if (cfdfcChannels.contains(channel)) | ||
| requiredOccupancy[channel] = static_cast<double>(latency) / targetII; |
There was a problem hiding this comment.
Is this a duplicate of the logic below?
| const ReconvergentPath &rp = pathWithGraph.path; | ||
| const CFGTransitionSequenceSubgraph *graph = pathWithGraph.graph; | ||
| const DataflowGraphNode &forkNode = graph->nodes[rp.forkNodeId]; | ||
|
|
||
| // We skip paths whose fork is outside all CFDFCs. | ||
| if (forkNode.type == DataflowGraphNode::REGULAR && | ||
| llvm::none_of(cfdfcs, [&](CFDFC *cfdfc) { | ||
| return cfdfc->units.contains(forkNode.op); | ||
| })) | ||
| continue; | ||
|
|
||
| std::vector<SimplePath> simplePaths = | ||
| enumerateSimplePaths(*graph, rp.forkNodeId, rp.joinNodeId, rp.nodeIds); | ||
| if (simplePaths.size() < 2) | ||
| continue; | ||
|
|
||
| // For each simple path, the sum of channel occupancies (N_c) and the total | ||
| // latency of the units. | ||
| std::vector<std::pair<LinExpr, double>> pathTerms; | ||
| for (const auto &path : simplePaths) { | ||
| LinExpr occupancySum; | ||
| double latencySum = 0.0; | ||
|
|
||
| for (NodeIdType nodeId : path.nodes) { | ||
| if (nodeId == rp.forkNodeId || nodeId == rp.joinNodeId) | ||
| continue; | ||
| const DataflowGraphNode &node = graph->nodes[nodeId]; | ||
| if (node.type != DataflowGraphNode::REGULAR) | ||
| continue; | ||
| auto latOrFail = | ||
| timingDB.getLatency(node.op, SignalType::DATA, targetPeriod); | ||
| if (succeeded(latOrFail) && *latOrFail > 0.0) | ||
| latencySum += *latOrFail; | ||
| } | ||
|
|
||
| for (EdgeIdType edgeId : path.edges) { | ||
| Value channel = graph->edges[edgeId].channel; | ||
| if (channelOccupancy.count(channel)) | ||
| occupancySum += channelOccupancy[channel]; | ||
| } | ||
|
|
||
| pathTerms.emplace_back(std::move(occupancySum), latencySum); | ||
| } |
There was a problem hiding this comment.
I couldn't convince myself that this logic corresponds to
Does it make sense to do something like:
for (auto cf : CFDFCs)
for (auto rp : reconvergentPaths) {
if (/* the BB sequence of "rp" is contained in the CFDFC */)
/* add occupancy balancing constraint */
}
2 participants
Problem: The implementation of the FPGA24 MILP's were missing constraint (10) and (11) described in this paper, resulting in patterns that are not in CFC's have a non-zero occupancy, which makes no sense, because occupancy tries to cater for incoming tokens in a loop body. (Initiation Interval).
This PR: Adds the missing constraints and re-works the rest of the code to adapt, as well as explicitly ignore patterns in the occupancy LP that have forks that are not part of a CFC.