[AIEPathfinder] Fix packet_dest-before-packet_source ordering crash (#2583)#2919
Merged
[AIEPathfinder] Fix packet_dest-before-packet_source ordering crash (#2583)#2919
Conversation
) Split the single if/else-if loop in runOnPacketFlow (AIECreatePathFindFlows.cpp and AIEPathFinder.cpp) into two passes: pass 1 collects the packet_source, pass 2 processes each packet_dest. This removes the implicit assumption that packet_source precedes packet_dest in the block, which MLIR does not guarantee. Tighten PacketFlowOp::verify() to enforce exactly one packet_source and at least one packet_dest, closing the verifier gap that previously allowed zero-source and multi-source flows through. Add a null-check after pass 1 in both transform passes as a defensive backstop against any future verifier bypass. Adds two lit regression tests: - packet_flow_dest_before_source.mlir: verifies correct output for dest-before-source ordering (single dest, multiple dests, keep_pkt_header). - badpacket_flow_source_count.mlir: verifies verifier errors for zero-source, multiple-source, and zero-dest packet flows. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in AIEPathfinderPass (issue #2583) where aie.packet_source appearing after aie.packet_dest inside an aie.packet_flow region caused uninitialized srcCoords/srcPort to be used, producing an assertion failure in Pathfinder::findPaths.
Changes:
- Two-pass refactor in
AIEPathFinder.cppandAIECreatePathFindFlows.cpp: pass 1 scans the region to extract the source (order-independent); pass 2 processes destinations using the already-known source, with a null-check backstop between passes. - Enhanced
PacketFlowOp::verify()inAIEDialect.cppto enforce exactly oneaie.packet_sourceand at least oneaie.packet_destin each packet flow region. - Two new regression test files covering the dest-before-source ordering fix and the verifier's rejection of zero/multiple sources and zero dests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/Dialect/AIE/Transforms/AIEPathFinder.cpp |
Two-pass loop fix to extract source before processing destinations |
lib/Dialect/AIE/Transforms/AIECreatePathFindFlows.cpp |
Same two-pass loop fix in the packet-flow transform pass |
lib/Dialect/AIE/IR/AIEDialect.cpp |
New verifier checks: exactly one packet_source, at least one packet_dest |
test/create-packet-flows/packet_flow_dest_before_source.mlir |
Regression test for dest-before-source ordering |
test/create-packet-flows/badpacket_flow_source_count.mlir |
Verifier error tests for zero/multiple sources and zero dests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Contributor
Coverage ReportCreated: 2026-03-04 18:16Click here for information about interpreting this report.
Generated by llvm-cov -- llvm version 18.1.3 |
Remove the extra `for (Operation &Op : b.getOperations())` line that was accidentally duplicated when writing the two-pass packet_source/dest fix, causing -Werror=unused-variable and cascading parse failures across all CI. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Update from '2024 Xilinx Inc.' to '2026 Advanced Micro Devices, Inc.' Co-Authored-By: Claude Opus 4.6 <[email protected]>
hunhoffe
requested review from
erwei-xilinx,
fifield,
jackl-xilinx,
jgmelber and
stephenneuendorffer
as code owners
fifield
approved these changes
Mar 4, 2026
hunhoffe
added a commit
that referenced
this pull request
Mar 12, 2026
PacketFlowOp::verify() was rejecting flows with more than one aie.packet_source, breaking valid fan-in topologies that predate the verifier check added in PR #2919. Relax the check from `numSources != 1` to `numSources < 1` so zero-source flows are still rejected while multi-source flows are accepted. The pathfinder (AIEPathFinder.cpp / AIECreatePathFindFlows.cpp) had a related "last source wins" bug: Pass 1 overwrote the single srcCoords/ srcPort accumulator on each PacketSourceOp, silently dropping all but the last source before calling addFlow(). This is left for a follow-up fix; the per-source loop change belongs in a separate commit. Tests added / updated: - badpacket_flow_source_count.mlir: remove multi-source rejection case (no longer an error), update error message for zero-source case - multi_source_packet_flow.mlir: positive verifier regression test - multi_source_pathfinder.mlir: pathfinder-level regression test (currently failing — documents the remaining bug) - npu-xrt/multi_source_packet_flow/: end-to-end hardware test Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[AIEPathfinder] Fix packet_dest-before-packet_source ordering crash (#2583)
AIEPathfinderPass assumed aie.packet_source always precedes aie.packet_dest in aie.packet_flow regions. MLIR provides no such guarantee, leaving srcCoords/srcPort uninitialized when the order is reversed and crashing via Assertion 'i < sb.srcPorts.size()' failed in
Pathfinder::findPaths.
Fix the assumption in both AIEPathFinder.cpp and AIECreatePathFindFlows.cpp by splitting the single-pass if/else if loop into two passes: pass 1 extracts the source unconditionally, pass 2 processes each destination with the already-known source.
Also closes a verifier gap: PacketFlowOp::verify() previously only checked that contained ops were of legal types, but did not enforce that exactly one aie.packet_source and at least one aie.packet_dest were present. Zero-source flows would hit UB in both passes;
multiple-source flows would silently use the last one. The verifier now rejects both cases with a diagnostic. A null-check after pass 1 in each transform serves as a defensive backstop.
Tests added: