fix(insert-sync): fix reuse ub insert sync bug#935
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 |
jimmychou0
changed the title
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
/run a3 |
|
已接收
页面会自动刷新，可以直接看当前阶段、排队情况和最近结果。 |
zhangstevenunity
left a comment
zhangstevenunity
left a comment
There was a problem hiding this comment.
Review: correctness-focused. No blocking issues.
This is a sound, well-scoped fix for the #934 class of bug: PlanMemory (level2) reuses a UB physical range across two distinct pto.pointer_cast roots, and the old MemAlias gated cross-root aliasing on i64 address-SSA identity (via GetRealRoot). So a TSTORE's async MTE3 read of stored and a later V-pipe write to the reused range were judged non-aliasing and no MTE3 -> V handshake was inserted.
Why it's safe. The change is monotonic: it only adds alias edges.
- For same-root inputs the new
hasKnownPhysicalAddressesbranch computes exactly the sameisBufferAddressRangeOverlapthe old same-root path did (adding the physical base to both sides is translation-invariant for themaxStart < minEndoverlap test), so results are identical. - The only behavioral delta is for different-root known-physical local pairs, where the old code always returned
false(roots are distinct constant SSA ->GetRealRootmismatch) and the new branch now returns a real range-overlap test.
So no previously-inserted sync is dropped, and the previously-missed cross-root overlaps are now caught. Placing the new branch before the rootBuffer == check is correct and necessary for the different-root case.
Fix reaches the real flow, not just the hand-written test. I traced the level2 lowering: AllocToPointerCast (in PTOPlanMemory) turns each memref.alloc into a single-address pto.pointer_cast whose operand is an arith.ConstantIntOp(_, 64) in vec space -- exactly the shape getKnownPhysicalAddress + isLocalAddressSpace recognize, so hasKnownPhysicalAddresses is set on the actual #934 buffers.
Regression test is valid. Without the fix, %stored(@1280, [1280,9472)) and %reused(@8192, [8192,16384)) have distinct constant roots -> old MemAlias returns false -> the set_flag/wait_flag(MTE3, V) lines are absent -> the CHECK-NEXT block fails. CI build-and-test (full lit) is green, so it passes with the fix and the existing suite is unaffected -- including multi_tile_const_slot_disjoint_sync.pto, whose disjoint slots stay disjoint under physical-range comparison (that test now passes for the right reason: range-disjointness rather than root-mismatch).
Note: I could not build/run locally to independently confirm the "fails without the fix" half -- the repo now requires LLVM 21 (post-PR796) and only an LLVM 19 prebuilt is available here -- so that half rests on the static reasoning above plus green CI.
Non-blocking observations
2. Coverage. The committed test deliberately hand-models the lowered form at --pto-level=level3 to preserve the explicit addresses (PlanMemory would otherwise reassign them). There is no end-to-end level2 test that lets PlanMemory actually create the reuse, and vpto-sim-validation / remote-npu-validation are skipped, so the real #934 numeric fix rests on the manual check quoted in the PR body ("k path becomes TSTORE -> set_flag(MTE3,V) -> wait_flag -> TMUL"). A level2 repro-derived test would harden this against future changes to PlanMemory's lowering shape.
4. Nit. BaseMemInfo::operator== (SyncCommon.h:116) was not extended with the new field. It is inert today -- the flag is a deterministic function of scope + baseAddresses, both already compared, so two infos cannot differ only in the flag -- but it is a latent footgun if the flag's derivation ever diverges from those inputs.
Overall: correct, minimal, and in the safe direction. LGTM modulo the optional symmetry/coverage items (see inline comments for 1 and 3).
| return static_cast<uint64_t>(address); | ||
| } | ||
|
|
||
| static bool isLocalAddressSpace(pto::AddressSpace space) { |
There was a problem hiding this comment.
Asymmetric coverage: alloc_tile(addr) never gets hasKnownPhysicalAddresses.
These two helpers are only wired into the pointer_cast path. But UpdateAllocTileOpMemInfo (line ~461) already extracts the constant address into baseAddresses (baseAddr), yet constructs its BaseMemInfo with the default hasKnownPhysicalAddresses = false.
At --pto-level=level3 PlanMemory is skipped and every pto.alloc_tile must carry an explicit addr (enforced in ptoas.cpp), and those ops survive to InsertSync -- lowerAllocTileOps in PTOViewToMemref.cpp:942 is [[maybe_unused]] and never called, and the translator has a live AllocTileOp handler. So two distinct level3 alloc_tiles that share or overlap a physical UB range would still miss exactly the cross-root MTE3 -> V handshake this PR adds for pointer_cast.
This is pre-existing and not a regression -- but for symmetry consider routing the same getKnownPhysicalAddress / isLocalAddressSpace treatment through UpdateAllocTileOpMemInfo so the reuse-race class is closed for both local-alloc materializations.
| llvm::errs() << " -> Comparing known physical local ranges.\n"; | ||
| if (a->baseAddresses.empty() || b->baseAddresses.empty()) | ||
| return true; | ||
| if (a->allocateSize == 0 || b->allocateSize == 0) |
There was a problem hiding this comment.
allocateSize == 0 now over-approximates cross-root.
When a known-physical local pto.pointer_cast has a dynamic shape, sizeInBytes stays 0, so allocateSize == 0 and this guard returns true -- meaning it aliases every known-physical buffer in the same address space, across roots. The old root-identity path returned false for such a buffer against a different root.
Direction is safe (more syncs, never fewer), but it is coarser than the previous behavior and, under event-id pressure, can turn into extra PIPE_ALL barriers. Low priority; flagging so it is a conscious tradeoff rather than an accident.
A3 板测完成（有跳过）
|
|
/run a3 |
|
已接收
页面会自动刷新，可以直接看当前阶段、排队情况和最近结果。 |
A3 板测完成（有跳过）
|
3 participants
问题根因
在
--pto-level=level2下，PlanMemory 会将不同的本地分配物化为不同的pto.pointer_cast。旧 InsertSync 对单地址pointer_cast主要依赖地址 SSA 的 root 身份判断别名，无法识别不同 root 对应的物理 UB 区间重叠。Issue #934 的根因是：PlanMemory 复用了 UB 物理内存，但 InsertSync 仍按 SSA 指针身份判断是否别名，导致漏掉同步。
具体过程：
PlanMemory 后：
stored = pointer_cast(1280) // 占用 [1280, 9472)
reused = pointer_cast(8192) // 占用 [8192, 16384)
两个区间在 [8192, 9472) 重叠。stored 在 tstore 之后，SSA 层面已经不再使用，因此 PlanMemory 认为其 UB 可复用给 reused。但 tstore 实际由 MTE3 异步执行，仍在从 stored 对应的 UB 区域读取数据。
随后 V pipe 的 tmuls 写入 reused，就可能提前覆盖 MTE3 尚未读完的 stored 数据：
TSTORE(stored -> GM) // MTE3 异步读 UB
TMULS(... -> reused) // V pipe 写入重叠的 UB 区间
旧 InsertSync 对单地址 pointer_cast 只记录相对偏移 {0}，并要求两侧的 rootBuffer 是同一个 SSA 值才继续做区间判断。pointer_cast(1280) 和 pointer_cast(8192) 的常量 SSA 不同，因此被误判为“不别名”，没有插入：
set_flag(PIPE_MTE3, PIPE_V, ...)
wait_flag(PIPE_MTE3, PIPE_V, ...)
结果是 MTE3 与 V pipe 存在数据竞争，表现为 tstore 输出偶发错误或被后续计算覆盖。
修复方案
pto.pointer_cast。pto.pointer_cast保留真实起始字节地址，不只保留相对偏移。验证
ninja -C /private/tmp/ptoas-issue934-pr-build ptoasllvm-lit聚焦回归：plan_memory_reused_tstore_sync.ptomulti_tile_const_slot_disjoint_sync.ptoissue870_identity_tmov_pruning.ptoTSTORE -> set_flag(MTE3, V) -> wait_flag(MTE3, V) -> TMULFixes #934