feat(pto): implement tpush/tpop direct TPipe flow#213
feat(pto): implement tpush/tpop direct TPipe flow#213KurrinQu wants to merge 1 commit intozhangstevenunity:mainfrom
Conversation
KurrinQu
force-pushed
the
feature/tpush-tpop-ops
branch
3 times, most recently
from
96533eb to
25c8b89
Compare
KurrinQu
force-pushed
the
feature/tpush-tpop-ops
branch
3 times, most recently
from
426433c to
13fad76
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30d617d5ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (auto tfreeOp = dyn_cast<TFreeOp>(&*it)) { | ||
| if (tfreeOp.getPipeHandle() == pipeHandle) | ||
| return true; |
There was a problem hiding this comment.
This early-exit treats any later pto.tfree with the same pipe as covering the current pto.tpop, which is incorrect when a block has multiple tpop operations on one pipe. In that case, an explicit tfree intended for a later pop makes earlier pops skip auto-insertion, so some consumed slots are never released on A5 and FIFO progress can stall. The check needs to distinguish per-pop balancing instead of a boolean per pipe.
Useful? React with 👍 / 👎.
| for (auto it = std::next(startOp->getIterator()), end = block->end(); | ||
| it != end; ++it) { |
There was a problem hiding this comment.
The last-read search only walks sibling operations in the same block, so reads inside nested regions (for example scf.if/scf.for bodies after the tpop) are not considered. When that happens, lastRead stays null and tfree is inserted immediately after tpop, releasing the slot before nested consumers run, which can corrupt A5 pipe usage. This needs region-aware read tracking rather than top-level block scanning only.
Useful? React with 👍 / 👎.
lib/PTO/Transforms/PTOToEmitC.cpp
Outdated
| if (fifoAddrs.empty()) | ||
| return rewriter.notifyMatchFailure( | ||
| op, "fifo_addrs must be present at EmitC lowering " | ||
| "(plan memory pass should have filled it in)"); |
There was a problem hiding this comment.
Verifier logic allows location=LOCAL with zero fifo_addrs, but EmitC lowering unconditionally fails when the operand list is empty. That makes IR accepted by verification fail later in conversion ("fifo_addrs must be present ..."), which is a pipeline regression for the documented optional LOCAL form. Either require the operand up front in verifier/pipeline modes that cannot materialize it, or implement the missing materialization before this check.
Useful? React with 👍 / 👎.
KurrinQu
force-pushed
the
feature/tpush-tpop-ops
branch
7 times, most recently
from
e13a435 to
4076a96
Compare
KurrinQu
force-pushed
the
feature/tpush-tpop-ops
branch
from
4076a96 to
a9da4f1
Compare
2 participants
No description provided.