feat(pto): implement tpush/tpop direct TPipe flow#213
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 👍 / 👎.
| 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
8 times, most recently
from
4076a96 to
a9da4f1
Compare
|
@codex review新版本 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a8e732c2f
ℹ️ 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".
| rewriter.create<TPopInternalOp>(op.getLoc(), declaredTile.getTile(), | ||
| op.getPipeHandle()); |
There was a problem hiding this comment.
In the PTOLowerTPopPass path I inspected, tpop_internal is emitted at the get_fifo_tile site rather than at the original pto.tpop site. Since pto.tpop is the blocking wait, this can reorder side effects: a sequence like tpop; <independent work>; get_fifo_tile is lowered so the independent work runs before the wait, changing synchronization timing and potentially breaking producer/consumer scheduling assumptions.
Useful? React with 👍 / 👎.
| if (!localAddr) | ||
| return rewriter.notifyMatchFailure( | ||
| op, "local_addr must be present at EmitC lowering " | ||
| "(plan memory pass should have filled it in)"); |
There was a problem hiding this comment.
initialize_l2g2l_pipe is defined/verified with optional local_addr, but EmitC conversion hard-fails when it is missing, so valid frontend IR using the documented optional form cannot be lowered. I also checked the surrounding pipeline and these init ops are only touched for liveness in memory planning, not rewritten to synthesize local_addr, so this path currently fails at conversion time instead of being supported.
Useful? React with 👍 / 👎.
KurrinQu
force-pushed
the
feature/tpush-tpop-ops
branch
2 times, most recently
from
a1b7b1a to
8376850
Compare
KurrinQu
force-pushed
the
feature/tpush-tpop-ops
branch
from
8376850 to
9eab9c4
Compare
* Add OP for TPartAdd & TPartMul * docs: add license headers to TPartAdd/TPartMul templates Add PR386 OAT.3 license headers to tpartadd_template.py and tpartmul_template.py. * fix-review: update license * Add OP for TPartAdd & TPartMul: update cmakelist --------- Co-authored-by: caojian5 <caojian5@huawei.com>
* Add OP for TPartAdd & TPartMul * docs: add license headers to TPartAdd/TPartMul templates Add PR386 OAT.3 license headers to tpartadd_template.py and tpartmul_template.py. * fix-review: update license * Add OP for TPartAdd & TPartMul: update cmakelist --------- Co-authored-by: caojian5 <caojian5@huawei.com>
* Add OP for TPartAdd & TPartMul * docs: add license headers to TPartAdd/TPartMul templates Add PR386 OAT.3 license headers to tpartadd_template.py and tpartmul_template.py. * fix-review: update license * Add OP for TPartAdd & TPartMul: update cmakelist --------- Co-authored-by: caojian5 <caojian5@huawei.com>
* Add OP for TPartAdd & TPartMul * docs: add license headers to TPartAdd/TPartMul templates Add PR386 OAT.3 license headers to tpartadd_template.py and tpartmul_template.py. * fix-review: update license * Add OP for TPartAdd & TPartMul: update cmakelist --------- Co-authored-by: caojian5 <caojian5@huawei.com>
* Add OP for TPartAdd & TPartMul * docs: add license headers to TPartAdd/TPartMul templates Add PR386 OAT.3 license headers to tpartadd_template.py and tpartmul_template.py. * fix-review: update license * Add OP for TPartAdd & TPartMul: update cmakelist --------- Co-authored-by: caojian5 <caojian5@huawei.com>
2 participants
No description provided.