feat: fixpipe tpipe design#883
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Codex Review该评论由 review 机器人自动更新。
SummaryReview failed at stage Findings未生成结构化 findings，因为 review 过程提前失败。 Log Tail |
HecreReed
force-pushed
the
codex/fixpipe-tpipe-design-draft
branch
10 times, most recently
from
57383b6 to
85f7319
Compare
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![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: 85f7319d98
ℹ️ 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".
| 推荐把 fixpipe 配置挂到 `initialize_pipe`： | ||
|
|
||
| ```mlir | ||
| pto.aic_initialize_pipe { |
There was a problem hiding this comment.
In the current frontend pipe flow, each data op resolves its id against an initialize_pipe in the same function, so a Vector-side tpop_from_aic will only see the peer function's aiv_initialize_pipe, not this Cube-side init. If the fixpipe attrs are only shown/defined on pto.aic_initialize_pipe, verifier rules that check the consumer result type/layout and any consumer lowering have no local pipe-level contract unless a new cross-function propagation path is designed. Please require the peer aiv_initialize_pipe to carry the same attrs (and verify equality), or document the cross-function lookup explicitly.
Useful? React with 👍 / 👎.
HecreReed
force-pushed
the
codex/fixpipe-tpipe-design-draft
branch
from
85f7319 to
9a54ecb
Compare
|
/run a3 rope_kv_cache |
|
/run a5 rope_kv_cache |
|
已接收
页面会自动刷新，可以直接看当前阶段、排队情况和最近结果。 |
A5 板测成功
|
HecreReed
force-pushed
the
codex/fixpipe-tpipe-design-draft
branch
from
9a54ecb to
11c96f4
Compare
|
已接收
页面会自动刷新，可以直接看当前阶段、排队情况和最近结果。 |
A3 板测成功
|
HecreReed
force-pushed
the
codex/fixpipe-tpipe-design-draft
branch
7 times, most recently
from
88a1fe2 to
7662777
Compare
HecreReed
force-pushed
the
codex/fixpipe-tpipe-design-draft
branch
23 times, most recently
from
3fb3eea to
6999392
Compare
HecreReed
force-pushed
the
codex/fixpipe-tpipe-design-draft
branch
from
6999392 to
8d733c6
Compare
|
/run a3 |
|
已接收
页面会自动刷新，可以直接看当前阶段、排队情况和最近结果。 |
A3 板测完成（有跳过）
|
zhangstevenunity
left a comment
zhangstevenunity
left a comment
There was a problem hiding this comment.
Review: fixpipe-over-tpipe implementation (full cross-layer)
Reviewed head 2418a9ab end to end — ODS (PTOAttrs.td/PTOOps.td), verifiers (PTO.cpp), frontend lowering, EmitC, manual, and the ~40 lit tests — cross-checked against the local pto-isa source. Overall this is a high-quality, faithful implementation; the design-doc iterations landed cleanly in code. Verified in particular:
- EmitC symbolic mapping is correct (
getFixpipeLayoutToken/getFixpipeQuantToken/getFixpipeReluToken): explicit symbol switches, never an ordinal cast. This matters because the new dedicatedPTO_FixpipeLayoutEnumordinals (NZ2ND=0/NZ2DN=1/NZ2NZ=2) do not match pto-isaLayoutMode_t(NZ2NZ=0/NZ2ND=1/NZ2DN=2) — an integer cast would have silently mappednz2nd→NZ2NZ. Good. - Type/layout/signedness helpers are faithful to pto-isa
FixpipeConsDType_tand to the design's src-type table, including the scalar-8bitsi8/ui8vs vector-8bitsi8-only split (correct —SET_QUANT_VECTORhas no<OutType>channel, unlikeSET_QUANT_SCALAR<OutType>). - A5 gating is enforced (
verifyFrontendInitCommon):qs322bf16_pre_*andqf322hif8/fp8are rejected on non-A5. - Effects modeling is complete (rule 18):
set_quant_*Write an id-partitionedFixpipeQuantStateResource;TPushOp/TPushToAivOpRead it — establishing the ordering dependency. - si8/ui8 carrier is real MLIR unsigned/signed
IntegerType(getEmitCScalarTypeTokenkeys on signedness; manual updated accordingly). - Frontend lowering propagates
acc_push_epilogue+ the peer-key attrs symmetrically; thePTOResolveReservedBuffersrelaxation to allow different local ids per peer is correct.
CI: build-and-test passes (all lit tests green). No blockers on the mechanical correctness.
One latent correctness issue worth fixing before the vector-quant reuse path is relied on, plus a coverage gap that lets it hide — details inline. Summary:
1. (P2, latent silent-miscompile) Scaling-tile liveness gap under vector-quant rematerialization. See inline on PTOPlanMemory.cpp. MemLivenessAnalysis sees the scaling tile's live range end at the original set_quant_vector, but EmitC rematerializes SET_QUANT_VECTOR(%fp) before every later reusing fixpipe TPUSH (the "bind once, reuse" pattern the design allows and vector_remat_a5.pto tests). SCALING is a normally-reused planned space, so under buffer pressure the tile's slot can be reused in the gap → the rematerialized SET_QUANT_VECTOR reads clobbered scaling data → silent wrong quant. Scalar reuse is safe (the payload is an f32 SSA value, not planned memory). I reached this by static analysis, not an executed repro — would appreciate a buffer-pressure A5 sim case to confirm/refute.
2. (coverage) Fixpipe has no executed validation. vpto-sim-validation and remote-npu-validation are skipped, and the A3 board run (OK 220) exercises the existing sample suite, not the new fixpipe lit files (FileCheck-only: IR verify + EmitC text match). So the A5-only modes and the whole EmitC/rematerialize/plan-memory interaction are validated only as text — exactly where finding #1 hides. Recommend at least one A5 sim/board run of a vector-quant fixpipe kernel with real buffer pressure.
3. (robustness, non-blocking) Peer-contract verification runs inside AivInitializePipeOp::verify() with moduleOp.walk() — see inline. Also verifyFixpipeConsumerType (from TPopFromAicOp::verify) duplicates the consumer element/layout checks AivInitializePipeOp::verify already does; both walk all tpops.
4. (minor) EmitC rematerializeFixpipeQuantBindings hard-fails the whole pass if peer resolution fails at EmitC time (resolveFixpipeConsumerTileType→findPeerFixpipeConsumerInit). It fails loudly, but it re-resolves the peer at codegen time rather than stashing the resolved consumer OutType during the earlier verified peer pass.
Design direction and the bulk of the implementation are solid — the substance is #1 (guard the vector-quant reuse liveness) and #2 (get one execution behind it).
| UpdateOpGenInfo(curOpInfo, llvm::to_vector(callOp->getOperands())); | ||
| OpKillHandle(curOpInfo, live, op->getBlock()); | ||
| } else if (isa<pto::TAllocOp, pto::TPushOp, pto::TFreeOp, | ||
| pto::SetQuantScalarOp, pto::SetQuantVectorOp, |
There was a problem hiding this comment.
Adding set_quant_scalar/set_quant_vector here marks their operands live only at the original op position — but that isn't sufficient for the vector case once EmitC rematerializes.
rematerializeFixpipeQuantBindings (PTOToEmitC.cpp) erases the original set_quant_vector and clones SET_QUANT_VECTOR(%scaling_tile) before every later fixpipe TPUSH that reuses the same-id binding — the "bind once, reuse" pattern the design allows and that test/lit/pto/fixpipe_frontend_emitc_vector_remat_a5.pto exercises:
set_quant_vector(%fp0){id=0}
tpush{id=0}
set_quant_vector(%fp1){id=1}
tpush{id=1}
tpush{id=0} // EmitC re-emits SET_QUANT_VECTOR(%fp0) here (CHECK line ~118)
In the IR, %fp0's only use is the original set_quant_vector. So MemLivenessAnalysis sees %fp0 dead after that op — including across the gap up to the 3rd tpush. SCALING is a normally-reused planned space (PTOPlanMemory.cpp:1752 goes through the same allocator as VEC/ACC, no pinning), so under buffer pressure another scaling tile can be assigned %fp0's slot in that gap. Then the rematerialized SET_QUANT_VECTOR(%fp0) before the 3rd tpush reads clobbered scaling data → silent wrong quantization.
The vector_remat_a5 test doesn't catch it: %fp0/%fp1 have overlapping original live ranges so they don't share a slot, there's no third scaling tile to force reuse, it's FileCheck-only, and vpto-sim-validation is skipped — so no execution ever runs the planner under pressure here.
Scalar reuse is safe: the payload is an f32 SSA value, not planned memory, and clones dominate.
Fix options:
- Extend the scaling tile's live range to the last reusing fixpipe TPUSH (treat those TPUSHes as pseudo-uses of the
set_quant_vectoroperand in liveness), or - Run the rematerialization before PlanMemory so the re-reads are real IR uses visible to liveness, or
- For v1, forbid vector-quant binding reuse across a gap — require an explicit
set_quant_vectorimmediately before each vector-quant TPUSH (scalar can still reuse).
I got here by static analysis, not an executed repro — a buffer-pressure A5 sim case would confirm.
|
|
||
| ImportReservedBufferOp matchedImport; | ||
| unsigned matchedImportCount = 0; | ||
| moduleOp.walk([&](ImportReservedBufferOp candidateImport) { |
There was a problem hiding this comment.
This peer-contract verification does a full moduleOp.walk() (plus lookupPeerFuncAcrossContainer and further walks of the peer function) from inside verify(). Two concerns:
- Cost/robustness: op verifiers run on every verification (after parse and, in expensive-checks builds, after every pass). Walking the whole module from each fixpipe
aiv_initialize_pipeis O(inits x module size) each time. This is generally discouraged for op verifiers, which are expected to be local. - The design doc (§9.1) itself frames peer-contract checking as a step done "while the frontend init is still visible" — which reads more like a dedicated verification pass than an op verifier. A pass would run once at a well-defined point, avoid the repeated full-module walks, and give clearer ordering guarantees.
Not a correctness blocker (parse-time verification does see the whole module), but consider moving the cross-function peer-contract resolution into a dedicated verification/prepare pass.
Separately: verifyFixpipeConsumerType (called from TPopFromAicOp::verify) re-derives and re-checks the consumer element type + layout that this init verifier already checks, and both walk all tpop_from_aic for the id. Worth consolidating so the two can't drift.
3 participants
Summary
tpipesupport of fixpipe-styleTPUSHtpushNotes
pto-isaTPUSH/TPOP/FixpipeParamssemantics with PTOAS frontend pipe IRTPUSH<Pipe, TileProd, TConfig>during EmitC