Conversation
There was a problem hiding this comment.
Pull request overview
This PR revises the Generate Code Pass to fix issue #16, which involves correcting the placement and timing of instructions in the code generation pipeline. The key changes reorder DATA_MOV instructions to proper time steps and index_per_ii buckets, introducing a new "egress" concept to handle register-before-link routing paths.
- Introduced egress instruction placement to handle cases where data moves through a register before being sent on a link
- Refactored mov expansion logic into smaller, composable helper functions for better maintainability
- Renamed variables from
tstotime_stepthroughout for improved code clarity
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/NeuraDialect/Transforms/GenerateCodePass.cpp | Core implementation: added egress signature tracking, MovRegSplit struct, splitMovRegs function, placeSrcEgress method, refactored expandMovImpl into helper functions, renamed ts to time_step, removed unused functions, extracted YAML/ASM tile emission into separate methods |
| test/e2e/histogram/histogram_kernel.mlir | Updated test expectations: DATA_MOV instruction moved from index_per_ii=3 to index_per_ii=0 with adjusted time_step and invalid_iterations; added new destination operand to ICMP_EQ and new DATA_MOV instruction |
| test/e2e/fir/fir_kernel.mlir | Updated test expectations: DATA_MOV instruction moved from index_per_ii=3 to index_per_ii=1 with adjusted timing |
| test/e2e/bicg/bicg_kernel.mlir | Updated test expectations: added new DATA_MOV instructions with proper timing, removed DATA_MOV from index_per_ii=11, consolidated operations into appropriate buckets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tancheng
left a comment
There was a problem hiding this comment.
Can you explain why there was an issue before, and how this PR resolves the issue? I originally thought this is a mapping issue, but it seems it is purely a codegen issue?
Yes. This was a codegen bug, not a mapping issue. ctrl_mov is meant to feed the reserve/state input of phi_start, but our consumer rewiring was matching on the wrong value (we used ctrl_mov’s source operand instead of the reserve operand). As a result, the rewrite didn’t apply and phi_start kept reading from a direction port (e.g. EAST) instead of a local register. |
Uh oh!
There was an error while loading. Please reload this page.