[Fix][Codegen][AIE] Fix Problematic Optimization on Stream Operations - #602
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets an AIE backend miscompile related to overly aggressive stream-operation lowering, particularly when FIFOs are reused across compute tiles (as described in issue #601). It refactors parts of the stream op handling in the AIE MLIR codegen and adds a focused regression test for multi-subtile (rho > 1) ATB dataflow.
Changes:
- Adjust AIE stream op lowering logic in
mlir_codegen.pyto avoid incorrect optimization in inter-compute-tile FIFO reuse scenarios. - Add a new AIE dataflow regression test covering
rho in {1,2,4}with bundling and forced unroll behavior. - Minor test refactor/rename in existing AIE matrix test to keep naming consistent.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
allo/backend/aie/mlir_codegen.py |
Refines stream op classification and FIFO selection logic during lowering to mitigate incorrect optimizations with FIFO reuse. |
tests/dataflow/aie/test_matrix.py |
Renames a region function (top1 → top) in an existing ATB GEMM test. |
tests/dataflow/aie/test_mapping_atb.py |
Adds a regression test for ATB mapping/bundling across multiple rho values to catch the #601 symptom. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
allo/backend/aie/mlir_codegen.py:449
inter_ct_fifoprocessing assumes every collected argument has at least one use (uses_[0]) and that the first use is a recognized stream op. If an argument is unused (or its first use is not memref.store/load/copy), this will raise IndexError or propagateis_put=Noneinto the fifo tuple selection. Add guards and skip empty/unrecognized arguments, and skip the fifo group entirely if no stream uses remain.
for arg in args:
uses_ = list(arg.uses)
is_put, is_tensor = check_stream_op_type(arg, uses_[0].owner)
uses.extend(uses_)
tests/dataflow/aie/test_mapping_atb.py:71
- This PR targets incorrect behavior for
rho > 1and the original report includes a failing case atrho = 8. Adding8here helps ensure the regression is actually covered for the worst-case subtile factor.
@pytest.mark.parametrize("rho", [1, 2, 4])
tests/dataflow/aie/test_mapping_atb.py:64
- If
df.build(...)ormod(...)raises,FORCE_UNROLL_INDEXwill remain set for the rest of the pytest session and can affect unrelated tests. Use try/finally and restore any prior value instead of unconditionally deleting.
os.environ["FORCE_UNROLL_INDEX"] = "1"
mod = df.build(top, target="aie", mapping_primitives=mapping_primitives)
mod(A, B, C)
del os.environ["FORCE_UNROLL_INDEX"]
tests/dataflow/aie/test_mapping_atb.py:111
- If
df.build(...)ormod(...)raises,FORCE_UNROLL_INDEXwill remain set for the rest of the pytest session and can affect unrelated tests. Use try/finally and restore any prior value instead of unconditionally deleting.
os.environ["FORCE_UNROLL_INDEX"] = "1"
mod = df.build(top, target="aie", mapping_primitives=mapping_primitives)
mod(A, B, C)
del os.environ["FORCE_UNROLL_INDEX"]
tests/dataflow/aie/test_mapping_atb.py:17
- This PR targets incorrect behavior for
rho > 1and the original report includes a failing case atrho = 8. Adding8here helps ensure the regression is actually covered for the worst-case subtile factor.
This issue also appears on line 71 of the same file.
@pytest.mark.parametrize("rho", [1, 2, 4])
Description
This PR partially fixes #601
Problems
Previous optimization on stream operation lowering is too aggressive and didn't consider inter-compute-tile fifo reuse. The leads to the bug mentioned in #601
Proposed Solutions
(What changes have you made)
Examples
Checklist
Please make sure to review and check all of these items: