Fix memory consistency non-inline metadata helpers#944
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the memory consistency pass to allow non-inlined function calls that contain payload memory accesses (such as loads and stores), provided that the memory consistency boundary operations (such as CMO, fence, and signal operations) remain in the caller. It introduces interprocedural mapping of callee payloads to caller arguments and relaxes the diagnostic check to only reject non-inlined calls that hide actual boundary operations. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
zhangstevenunity
left a comment
There was a problem hiding this comment.
Reviewed the memory-consistency refactor end to end: traced both new lit tests through the pass and cross-checked the doc HW-mapping claims against PTOToEmitC. Overall the change is sound for its intended cases; one substantive concern is inline.
The red build-and-test looks like a pre-existing flake, not this PR. The failing test is fixpipe_frontend_emitc_nested_quant_block_a5.pto, which this PR does not touch (byte-identical to main) and which contains no func.call/CMO/fence/signal ops, so pto-memory-consistency leaves it unchanged. Evidence it is nondeterministic: the same main tip f0bdc1ee (this PR's merge base for CI) failed build-and-test on the 2026-07-15 run, crashing in a different a5 fixpipe test (fixpipe_frontend_emitc_scalar_remat_a5.pto, empty stdout), but passed build-and-test on the 2026-07-16 run. Both failures are ilist_iterator ... isKnownSentinel / empty-output crashes in the a5 fixpipe scaling-remat lowering, i.e. a latent nondeterministic use-after-free in that path, unrelated to this diff. Recommend re-running CI for a green bar before merge, and filing the a5 fixpipe flake separately.
Logic is correct for the intended shapes. getReleaseStateForCall + remapCalleePayloadToCallOperand correctly thread callee MTE3/FIX payload writes back to the caller operands, and both new tests produce the expected sequences (call_hidden_payload_write_with_marker -> pipe_barrier(PIPE_MTE3); dsb(DSB_DDR); ..._with_internal_barrier -> dsb only, no redundant MTE3 barrier). Recursion cycle-guarding via activeCallees is correct; single-block callees get precise internal-barrier handling while multi-block callees are handled conservatively (safe). Docs match the lowering (fence.barrier_all<gm|all> -> dsb(DSB_DDR); whole/single-line CMO -> dcci ENTIRE/SINGLE_CACHE_LINE).
One concern (inline, non-blocking): the narrowing makes the release path interprocedural but leaves the acquire path blind to loads inside now-allowed non-inline callees. Details on isMemoryConsistencyBoundaryOp.
| return macroState.drainMte2 || macroState.drainMte3 || | ||
| macroState.drainFix || macroState.needsDsbDdr || | ||
| macroState.needsGmCacheCmo; | ||
| static bool isMemoryConsistencyBoundaryOp(Operation *op) { |
There was a problem hiding this comment.
Narrowing the fail-fast to boundary-ops-only introduces a release/acquire asymmetry that can silently drop an acquire diagnostic.
Before this change, a non-inline callee containing a GM load_scalar/store_scalar was rejected here (the old isMemoryConsistencyRelevantDirectOp returned true for GM scalar load/store), which forced the helper to be inlined so the acquire walk could see its loads. Now such a callee is accepted, but only the release path was made interprocedural (getReleaseStateForCall summarizes callee payload producers into the caller). The acquire path was not: annotateSignalAcquire / collectSignalAcquireState / annotateSignalAcquireForBlock have no func.call handling, and every function is analyzed independently starting from an empty SignalAcquireState.
Consequence:
%ok = pto.comm.ttest ... // caller acquire -> pendingInvalidateGmCache
func.call @helper(%ctx) // @helper does a cacheable GM load_scalar
The cacheable load inside @helper is now reached after an acquire, but diagnoseAcquireLoad never fires for it: @helper is analyzed with an empty acquire state, and the caller's acquire state does not cross the call. Previously this program was rejected (forcing inline, after which the load would be diagnosed as needing pto.cmo.cacheinvalid all #pto.address_space<gm> before the cacheable load). So a program that should error now compiles silently -> potential stale GM read after acquire.
The motivating metadata helpers (load_scalar on CommContext) are legitimately exempt, but nothing constrains a now-allowed non-inline callee to metadata-only loads. Is this asymmetry intentional? If so, it would be good to pin it with a regression test and note it in the design doc; otherwise the acquire path likely needs the same interprocedural treatment as the release path. Either way it is currently untested.
Summary
pto-memory-consistencynon-inline call diagnostics to hidden memory-consistency boundary ops (CMO,fence,TNotify,TWait,TTest).load_scalar/store_scalar, so metadata helpers such as remote-offset readers are not forced to inline or add unrelated CMO markers.cmo.cacheinvalid %payloadmarkers can still drive precise pipe drain insertion.Motivation
CommContextwithload_scalarto compute remote offsets.Design
cmo.cacheinvalid %payloadmarkers to still trigger required MTE/FIX drain insertion.Testing
PTOMemoryConsistency.cpp.o.ptoas.test/lit/pto/memory_consistency_noninline_metadata_call.ptotest/lit/pto/memory_consistency_noninline_release_call.ptotest/lit/pto/memory_consistency_noninline_call_invalid.ptogit diff --check.Risk / Rollback