Unified allocator : Mixing bufid and event id into a unique design - #1174
Open
Adamkh329 wants to merge 34 commits into
Open
Unified allocator : Mixing bufid and event id into a unique design#1174Adamkh329 wants to merge 34 commits into
Adamkh329 wants to merge 34 commits into
Conversation
Three checks over the synchronization a compilation emits, each judging one run on
its own rather than against a reference:
G3 every static event and buffer id is one the target permits for that op,
direction and arch, and set/wait stay balanced per direction. Rotating
ids on set_flag_dyn / wait_flag_dyn resolve at runtime and are not
checked.
G2 for a given id key the live intervals are pairwise disjoint. Event ids
are keyed by (srcPipe, dstPipe, id) because event flags are
per-direction disjoint hardware, so EVENT_ID0 may be live in MTE2->V and
V->MTE3 at once; keying on the id alone would raise a false violation on
correct code.
G1-self every dependence the front-end analysis reports is ordered by emitted
sync, derived from DepBetween directly. Pairs on mutually exclusive
scf.if arms, and on A5 PIPE_V pairs the target orders itself, are
excused; both exclusions are counted and reported rather than dropped.
All three are read-only apart from diagnostics and off by default. They run
against the existing sync path and need no new allocator.
A self-validation fixture pins that each of three fault classes is caught by
exactly one gate and is silent on the other two, so no gate is redundant and none
is vacuous. Two of those classes cannot be written as input IR -- pto.set_flag
spells its id as an EVENT_ID0..EVENT_ID7 enum and get_buf's id is verified into
[0, 31] -- so they are appended to the gate's own record list after extraction,
never to the IR, behind a hidden flag. An unrecognised selector fails loudly,
because injecting nothing would let the test pass while proving nothing.
Outside the new files this adds a public accessor,
SyncEventIdAllocation::GetReservedEventIdNum, so the gate reads the same
per-direction reservation map the sync pass obeys instead of duplicating it.
No cycle-time change: the gates emit nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`get_buf`/`rls_buf` name their pipe with any of the three spellings `PTO_PipeLikeAttr` admits. G1's coverage walk decoded only two of them and dropped the op on the third, so a valid buffer-token chain established an ordering the gate then reported as uncovered while G2 and G3 accepted it. Read the pipe through the extractor's decoder, which already handles all three and mirrors `verifyBufSyncOp`, so the two views cannot disagree on which spellings exist.
The same chain over the same two pipes, written once as a plain PipeAttr and once as a pipe_event_type. Either input alone would pass a gate that decoded only that spelling, so the pair is what makes the property testable. G2 and G3 are pinned on the same input as well: a decode gap in one gate shows up as two gates accepting what a third rejects.
G2 paired set_flag with wait_flag in flattened walk order, so a pair split across the two arms of an scf.if balanced the scan and passed: the counts match and the stack pops the wait against the set, while no runtime path executes both. The same blindness passed a conditional set with an unconditional wait, which hangs, and an unconditional set with a conditional wait, which leaks the id. A pair must now sit under the same enclosing conditionals. Loop nesting is deliberately not compared: a set outside a loop priming a wait inside it is the intended idiom and accounts for most emitted pairs, and the zero-trip case is already checked against the loop spine. G3 stays unchanged. Its balance check counts set and wait ops per direction, and a split pair has one of each, so a count cannot see this; the gates are meant to catch each fault class in exactly one place.
Five placements of one event id: the pair split across the two arms of an scf.if, a guarded set with an unguarded wait, its mirror, and two that are legal. Every run is `not`, so removing the rule makes the pipeline succeed and the legal cases fail too rather than passing vacuously. The last function is the bound on the rule. Its set sits outside a loop and its wait inside, so the halves are in different blocks while carrying the same conditional guards. That is the shape the emitter produces most, and it is what fails if the comparison is ever tightened to blocks.
G1-self demanded synchronization for a PIPE_S -> PIPE_S pair that neither allocator in this tree emits: IsNoNeedToInsertSync returns before running the dependence analysis, and GraphSyncSolver's same-pipe barrier path returns for PIPE_S unconditionally while gating PIPE_V and PIPE_M on the arch. An existing test, scalar_gm_tstore_sync.pto, compiles and runs while the gate rejected it. The exclusion is arch-independent, because that is how both allocators key it, and counted in its own field rather than folded into arch_guaranteed: no ISA or design document states what ordering the scalar pipe gives its own ops, and pipe_barrier(PIPE_S) is expressible and reaches the emitted code, so this rests on compiler behaviour rather than on an architectural guarantee. The comment at the exemption says so. No production kernel is affected: all 121 corpus functions report pipe_self_ordered=0 and the uncovered set is unchanged.
…s reached G1's edge filter keyed on program order alone, so a sync inside one arm of an scf.if was credited with ordering anchors outside that arm. The false path executes both anchors with nothing between them, and the gate reported the kernel covered. An edge is now credited only when the conditionals enclosing the consuming end of the mechanism -- the barrier, the wait_flag, the get_buf -- are a prefix of those enclosing the sink, so the sync is reached on every path that reaches the sink. Enclosing loops are deliberately not compared. Structural dominance would say an op in a loop body never dominates an op after the loop, which withdraws credit from a barrier in a constant-trip-count loop that always executes: on control_flow_nested_vec that rejection alone reports three same-pipe dependencies as uncovered when the ordering does hold. A loop whose trip count can be zero is a real hole and is not covered here; the carried rules test the loop spine for it. Corpus: no kernel changes verdict. Two kernels already reporting uncovered gain violations that were previously credited to a conditional barrier -- for_break_like_kernel 6/12 to 3/15 and while_break_kernel 15/3 to 12/6.
Four placements of one barrier. The defect: barrier on one arm, both stores outside it, which the gate used to report covered. Two legal cases hold the rule in from either side -- the same barrier hoisted out of the `if`, and a barrier and sink together in one arm, which a rule keyed on the source side would reject. The fourth is the bound. Its barrier sits in a constant-trip-count loop with the sink after the loop, so it always runs, but structural dominance says a body op does not dominate an op after the loop. That line fails if enclosing loops are ever compared, which is why they are not. Every run is `not`, so dropping the rule makes the pipeline succeed and the legal cases fail too rather than passing vacuously.
A bare `pto.set_flag` in a function whose other ops are loads and stores leaves the enclosing segment's section kind ambiguous, and normalization now rejects that rather than guessing. Both inputs wrap their bodies in `pto.section.vector`. Gate output is unchanged -- all five verdicts in the event-pair file and all four in the reachability file are byte-identical to before the wrapping, because a section carries no conditional and the walk still sees the same ops in the same order.
G1 and G4 compare one compilation against a reference compilation of the same
function, so they answer a question the absolute gates cannot: does this run
establish everything a known-good run established?
G1 every happens-before ordering in the reference must also hold here. Cannot
be cleared by a compilation that emits nothing, which is the fault class the
count floor is blind to.
G4 sync-op counts must not regress, per class rather than in total. On the
shared fixture the barrier-all mode replaces two directed pairs with five
full-core barriers for an IDENTICAL total of 5, so a scalar floor would wave
a fully serializing allocation through; only the per-class rules catch it.
Both are read-only, off by default, and driven as two runs: --dump-sync-coverage
and --dump-sync-counts write a reference profile, --check-sync-coverage and
--check-sync-count-floor compare against one. What the reference is compiled with
is the caller's choice.
Each gate is pinned against the fault it exists to catch: G1 against a
compilation with no synchronization pass, reporting all three orderings lost by
anchor pair; G4 against the barrier-all degenerate and against a reference that
does not mention the function, which is a violation rather than a skip.
The buffer-token rule is shown to be load-bearing rather than asserted: on A5 the
buffer-id pass discharges the same hazards with get_buf/rls_buf and its edge set
is a strict superset of the event reference's, which only holds because rls_buf is
credited as ordering the following get_buf.
No cycle-time change: the gates emit nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ne model The existing passes each own one mechanism, so neither can trade one resource against another: when a pipe direction exhausts its event pool, insert-sync must serialize with a PIPE_ALL barrier even where buffer-id tokens are free. --enable-unified-sync treats the three mechanisms as resource classes over a single interval-colouring problem. Loop-carried compensation is synthesised before any id exists, since an id assigned to an op that later moves is worse than no id. Buffers whose direction would overflow its pool are routed to tokens where the buffer is eligible, which shrinks the colouring problem. What remains is coloured per direction by live interval, reusing an id across disjoint intervals. Anything that still does not fit spills to a barrier rather than reaching emission unallocated. The dependence analysis, hoisting and redundancy pruning are SHARED with pto-insert-sync rather than reimplemented. That is a constraint, not a convenience: both passes see the same program and the same hazards, which is what makes a differential comparison between them meaningful. Routing is per buffer and all-or-nothing. A hazard holding a token on one side and an event id on the other is split across two mechanisms, and a split pair hangs, so mechanism is a property of the hazard and is stamped on both halves together. Hazards are joined to buffer-id clusters with MemAlias. A tuple of scope, base address and size cannot serve: addresses are unassigned at that point, so the key collapses and distinct clusters share it. Adds SyncOperation::depMemInfos, the BaseMemInfo objects behind a dependency, so a consumer can recover which tile a hazard is on. The root buffers already stored are the root ALLOCATION, coarser than a buffer, and several buffers share one. Correctness is checked by gates over the extracted sync rather than by comparing output against the incumbent: coverage against a reference profile and against the dependence analysis directly, non-interference, device-id legality, a per-class sync-op floor, and a reservation check for the ids a macro's library implementation consumes internally, which no other gate can see. K is derived from the architecture: 0 on A3, which leaves the buffer-id class empty and the routing step inert, and 32 on A5.
…tives Nine lit files, each pinning one behaviour and each shown to fail when that behaviour is deliberately broken. No real kernel exhausts an event pool, so the overflow, routing and spill paths have no natural input. Synthetic kernels supply one, forced by shape rather than by a pool knob: overflow == max(0, N - 8) in the number of simultaneously live hazards. A non-overflowing control is pinned beside the overflowing case, so a kernel that is simply broken cannot be mistaken for a saturated one. One overflowing kernel is written back and routes to tokens; another is a pure forward chain, never written back, so it must spill instead. Together they stop overflow alone from being read as a routing condition. The remaining files pin the data model and that hazards on different buffers report DIFFERENT clusters, which is what separates the MemAlias join from a degenerate one; that mechanism is derived from type rather than defaulted flat, is stamped on both halves of a hazard, and falls back to barrier together with the type on spill; both extractor renderings, their field names and their program order; rotation at depth two, where both ids must be primed ahead of the loop or the first iteration landing on a nonzero slot blocks forever; that a macro's library-internal ids are reserved before colouring, with the reservation count pinned separately because the emitted id alone would be satisfied by luck; that the G2 compensation exemption scopes to a hazard's own pair and no further; and that G2 still reports a collision injected into its own record list, in a shape that cannot be written as input IR because it exists only before codegen.
Covers the intent, the three mechanisms and their pools, the flag surface, where the pass sits and why that slot is forced, the data model, the gates, the testing inventory, and ten known limitations. Two things a reader needs that are easy to miss. The allocator does not reproduce the incumbent op for op, but it matches it in count everywhere: of 130 sample kernels, 115 emit under both modes, 56 byte-identical and 59 differing, every difference at an identical per-kernel count and equal totals on both sides. It is therefore cycle-neutral on the production corpus rather than an improvement to it, because on a kernel that never exhausts a pool there is nothing for a second mechanism to win. The limitations state which side each gap is on -- the allocator or the gate that checks it -- and whether the incumbent shares it. Where a limitation cannot be settled from this repository, it says so and says what would settle it.
…cator A dead-producer sweep over everything this stack adds found five members with only one live side. None is a correctness defect, but each is the same shape as a field that is declared and consumed yet never written, which is invisible to the compiler, the test suite, the gates and a differential comparison alike. Three coloring counters were incremented on live paths and never read: skippedRouted, rotating and idsAssigned. Their own comments say they exist so a count is visible rather than folded into another, which was not true of any of them. They are now printed in the coloring report, where they discriminate: skipped_routed equals the hazards a routed buffer took off the colourer, rotating counts hazards holding more than one id, and ids_assigned exceeds assigned exactly when a hazard is multi-buffered. SyncOpRecord::mechanismCode was written and never read; the string form beside it carries the same value and is the one the report prints. Removed. Interval::length() had no caller and no natural one. Removed.
The equivalence section argued cycle neutrality from sync-op count identity. Those are claims over different domains: equal op counts do not entail equal cycles, because the orderings still differ and ordering is not free. Restated as three claims of decreasing strength. Op-count parity is proven across the sample corpus, on A5 as well as A3, at a delta of zero. Two A5 kernels were then measured in the cycle domain: rmsnorm costs +0.16% and a cube kernel costs nothing, with a determinism control showing the former is signal rather than variance. Cycle neutrality in general is explicitly not claimed, and nothing here rests on it. Records that the two available cycle metrics disagree in direction on rmsnorm -- summed instruction cycles fall while per-core latency rises -- and which one is authoritative, since quoting the other would report a small regression as a win. Three further places stated a measurement over the sample corpus as a wider property: counts matching "everywhere", "the real corpus" not overflowing a pool, and the A5 same-pipe exemption resting on an absence of emitted sync as though it were a target guarantee. All three are now scoped to what was measured.
…ill relies on it `ResourceModel::barrierIsUnbounded` recorded that barrier is the unbounded spill class -- the property that makes the allocator total -- but nothing referenced it, so it documented an invariant without holding anything to it. The spill in `colorEventIds` is where that totality is assumed: it hands the hazard to a barrier and continues, with no failure branch. Assert it there, so a bounded barrier class cannot be introduced without this stopping compiling until a failure mode is written.
The comment was copied from `typeCode` and described a field that does not exist: it claimed `static_cast<int>(SyncOperation::MECHANISM)` with a -1 unset sentinel, and told gates to switch on it "never on the `mechanism` string" -- while being that string. An earlier revision did carry a `mechanismCode`; it was replaced by the name and the comment did not follow. State what it is: the mechanism name, filled by the pre-codegen extractor, used for rendering. Nothing switches on it, and the reason -- no numeric companion -- is recorded along with what to add if a gate ever needs the mechanism as a decision.
…ir element The differential gates' syncops leak scan read `open.first` off an entry of `LiveIntervals::opens`, which is a struct with named fields rather than a pair. The two changes sit in different regions of the file, so the merge that brought them together reported no conflict and produced code that does not compile. Nothing else in the file reads an interval that way; the remaining `.first`/`.second` uses are map entries.
… pins G1-self now reports a `pipe_self_ordered` count beside `arch_guaranteed`, so four pinned report lines no longer match. Every other field is unchanged, verified against a fresh run of each: the inputs contain no conditional at all, so the reachability rule that accompanies the new field withdraws no coverage edge and the claims these pins exist to hold -- 139 covered under the unified allocator against 177 under InsertSync, and the 66/28 split of ordering credited to the target -- are untouched. One pin in sync_event_rotation.pto stops before the new field and needs no change.
…ents Six comments stated things the surrounding code contradicts: - `bufferClusters` was documented as joined on `TileKey`, which is not a type in this tree. The join is per `BaseMemInfo *`, by pointer identity with a `MemAlias` fallback. - `Alpha` was documented as read by nothing. `Alpha::forHazard` is called from `printSyncModel`; what is true is that no allocation decision reads it -- `routeBuffers` tests `Buffer::isWrittenBack` directly. - The `end < start` clamp was documented as deleted. It is still there, reachable when `GetForEndIndex` is set but does not resolve to a loop element. - `forceMechanismOnFirstHazard` justified stamping both halves by claiming `GetMatchSync` cannot carry a mechanism and never runs again. It does carry one, and `synthesizeLoopCompensation` calls it. Both halves must still be stamped, but the reason is that the pair was built before any mechanism was chosen. - The syncops dump was annotated as carrying no event ids because no allocator had run. Extraction runs after `colorEventIds`, so the ids are present. - The macro-collision check was annotated as reporting against a reservation that is not built yet. `seedHiddenMacroEvents` runs before colouring and `colorEventIds` already refuses a reserved id. Also drops measured figures that no test pins and that state no invariant. No functional change. Emitted IR is byte-identical to the parent across all 130 sample kernels on both the insert-sync and unified paths, and the G1 report is unchanged (124 lines, 109 OK, 15 notOK). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cuts the long blocks to what states an invariant or a hardware constraint, dropping design narrative and rationale that reads out of the code. Also repairs the structure the volume was hiding: - `colorEventIds` had no doc comment; the algorithm description, the reporting conventions and `seedHiddenMacroEvents`' note had all merged onto `seedHiddenMacroEvents`. Each is now on the declaration it describes. - `SyncModel`'s summary line sat above `HiddenReservation`. - `SetEventIds`' description sat above `demand()`. - One sentence in `synthesizeLoopCompensation` restated itself verbatim. - A block describing macro-event reservation was attached to no code. - The predicate-divergence note named the input file rather than the function the report prints, which is `prefetch_disjoint_slots`. Comment-to-code ratio moves from 1.80 to 1.80 in the header, 0.47 to 0.36 in UnifiedSyncModel.cpp and 0.52 to 0.42 in PTOUnifiedSync.cpp, against 0.29 for SyncOracleGates.cpp and 1.35 to 2.26 for the two oracle headers. Comments only. Stripping comments from all three files gives output identical to the parent, emitted IR is byte-identical across the 130 sample kernels, and the G1 report is unchanged at 124 lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by re-auditing every comment against the code it describes, with each candidate checked by two independent reviewers before being changed. Four were introduced by the preceding comment compression, which is the risk that pass carried: - The interval was documented as `[beginId, endId)`. `buildSyncModel` uses `endId + 1`, and the implementation comment says so; the extra index covers the tail-wait sitting at endId. - The colourer was documented as always taking the lowest free ids. It skips the most recently freed id while another is free. - The synthesised head-set was documented as landing in the carrying loop begin's `pipeBefore`. It lands at the anchor `hoistAnchorFor` returns, which may be an earlier op in that loop's enclosing region. - `accessRange` was documented as accesses only with no loop widening. It unions hazard intervals, and a loop-carried hazard's interval is its whole carrying loop. Three were older: - `Buffer::smallestPool` is the event pool of the direction the peak came from, not the smallest pool among the buffer's directions. - The coverage closure was documented as tri-valued with `1+1` dropped. Distances compose additively and are dropped only above `kMaxDistance`. - G1-self's exclusions were described as two in both the pass description and the flag help. There are three: `scf.if` arms, `PIPE_S`->`PIPE_S` on every arch, and A5 `PIPE_V`->`PIPE_V`. Each has its own counter. The design document's uncovered-dependency figures were stale by exactly the six the reachability fix added: 433 and 429, not 427 and 423, measured in both modes. No behaviour change. Comments and two help strings only; stripping comments leaves the three source files identical to the parent, emitted IR is byte-identical across the 130 sample kernels, lit is 1661 of 1662, and ctest is 50 of 50. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two section banners sat stacked with no code between them, and the first named the wrong gate for what followed: everything under it -- checkCoverageSuperset, parseCoverage, printCoverageViolations -- takes a reference profile and is G1 differential, while computeSelfCoverage sat 250 lines further down under no banner at all. The titleless divider already present above the G1-self helper namespace is that banner's stranded closing rule. Titling it restores the file's divider/title/ divider convention and puts the heading on the section it describes, without splitting the helpers away from the function that is their only caller. Comments only; stripping comments leaves the file identical to the parent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…token cannot express Two ways a routed hazard could lose its ordering with nothing reporting it. TOKEN SITES NOW COME FROM THE ALLOCATOR'S HAZARDS. `BufidSyncAnalysis::collectDependencies` enumerates FORWARD pairs only, so a loop-carried hazard -- whose producer sits after its consumer in program order and whose ordering crosses the back edge -- can never appear in `depPairs_`. Routing was decided over the allocator's hazard set and emission driven from that other set, so a hazard present in one and absent from the other had its event ops suppressed and no token emitted in their place. A token does carry such an ordering, and needs no priming pair to do it: the counter starts free, so iteration N+1's `get_buf` blocks on iteration N's `rls_buf`. Only the emission was missing. Sites are now derived per routed hazard, bracketing BOTH endpoints -- four anchors where the producer and consumer are distinct ops. Where several ops on the right pipe alias the cluster, all of them are bracketed rather than one being chosen: over-bracketing spends a token, under-bracketing loses an ordering, and those are not symmetric. `bracketed_hazards` / `unbracketed_hazards` make the completeness checkable instead of assumed. ROUTING NOW REFUSES A HAZARD NO TOKEN CAN EXPRESS. `Hazard::tokenExpressible` records the four shapes that cannot be realised as a token: same-pipe, which needs a barrier because a token names a buffer and not a pipe pair; no aliasing clique, so there is no token to take; GM or endpoints in different scopes, which is why `BufidSyncAnalysis` excludes them; and a pipe `BufidSyncCodegen::mapPipelineToSyncOpType` has no name for, which fails the compilation rather than degrading. Checked over every hazard on the buffer, since routing is all-or-nothing, and reported as `not_expressible`. Both apply on the default path. The overflow path had the same latent gap and had only escaped it because no production kernel routes. No behaviour change where routing already fired: `overflow_writeback` and `overflow_id_gap` emit the same 24 anchors, 24 get and 24 rls as before, now hazard-derived and provably complete, with G1-self uncovered=0 and carried=115/115. Corpus emission is byte-identical across all 130 sample kernels; lit is 1661 of 1662 and ctest 50 of 50. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
I reviewed exact head
Validation performed:
|
…an address buildSyncModel already derives what a hazard sits on: its buffer clusters, whether a mirror-direction partner shares one, whether a token can express it at all. It could not say whether a hazard exists only because two DISTINCT allocations were placed at one physical address, which under --pto-level=level3 the caller chooses and PTOPlanMemory never revisits. No SSA value flows between two distinct allocations, so such an ordering is invisible as dataflow and shows up only as synchronization the kernel pays for. Hazard::addressSharing records it, classified by tile type: identical types are one buffer holding successive values, differing types are a reshape or transpose view over live data. The shared slot and both allocations, in program order, are kept beside it so a reader can name the two sites. INFORMATIONAL ONLY. No routing, refusal, mechanism choice or emission reads it. The type test is a heuristic: two same-typed tiles can be aliased deliberately, and the ordering between them is then a real read-after-write through memory. A decision taken on a wrong classification would drop a live ordering, so the field is reported and never acted on. The address is read from the pto.alloc_tile operand rather than from BaseMemInfo::hasKnownPhysicalAddresses. That flag is documented as separating PlanMemory-materialised addresses from root-relative offsets, but nothing in the tree ever sets it true and its only reader is therefore dead on every path; keying on it would have made the classification silently vacuous. Surfaced in the existing model report: share= per hazard, share_reuse and share_alias on the kernel line, so the classification is visible without a dedicated flag. Emitted IR across the 124 compiling corpus kernels is byte-identical to 6307327. lit is 1661 passed of 1662 with one unsupported and none failing; ctest is 50 of 50. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ess share forces Reads Hazard::addressSharing and re-derives nothing. Prints, per hazard, the shared scope and address, the pipe direction, and both allocations with their source locations, so a reuse can be traced to the two sites that chose the address. Counts reuse and alias-view separately: totalling them would attribute recoverable cost to orderings that carry real dataflow through an alias and cannot be removed at all. Off by default and report-only. Emitted IR across the 124 compiling corpus kernels is byte-identical with the flag on, with the flag off, and at 6307327 before the classification existed. The fixture pins one of each class in a single kernel, and pins that the flag off produces no report surface. The two orderings are given different pipe directions deliberately: with the same direction, program order lets one sync cover both and RemoveRedundantSync keeps only one, which would leave a classification untested. Explicit addr operands are accepted only under --pto-level=level3, so that is the only level at which anything is reported. lit is 1662 passed of 1663 with one unsupported and none failing; ctest is 50 of 50. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four of the five automatic synchronization modes were individually refused alongside pto.tassign; unified sync was omitted from that list. The mutual-exclusion check above it counts unified sync, but that only rejects enabling two modes at once and says nothing about tassign. pto.tassign rebinds a tile's address at run time. The memory-dependence analysis every one of these modes shares does not model that rebinding, so a mode which accepts the op emits synchronization computed against addresses the kernel does not use. Unified sync runs on exactly that analysis, so it needs the same refusal as the other four. Before: --enable-insert-sync, --enable-bufid_sync, --enable-graph-sync-solver and --enable-inject-barrier-all-sync each exit 1 with a diagnostic on tassign_level3_loop_rebind.pto, while --enable-unified-sync exits 0 and emits a kernel. After: all five exit 1. The mode-free path still compiles the file unchanged, so the guard is not over-broad. The regression test goes in the file that already pins the insert-sync refusal, as a fifth RUN line, so the next mode added is compared against a list that is visibly complete. lit is 1662 passed of 1663 with one unsupported and none failing; ctest is 50 of 50. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…serting first The guard for a set/wait hazard reaching codegen with no event id set its flag, then executed assert(false), then emitted the diagnostic. With assertions compiled in, the abort happens first and the diagnostic never appears, so the negative test that pins the message sees a crash. The assertion defeated the error path it was meant to harden. Removing it loses nothing. sawUnrealisedHazard_ is set immediately above and the unified pass turns it into signalPassFailure, so the condition still fails the compilation, and it now does so identically whether assertions are on or off. The insert-sync path cannot reach the guard at all, which is unchanged. Verified under an LLVM 21.1.8 assertion build (LLVM_ENABLE_ASSERTIONS=ON, LLVM_ENABLE_ABI_BREAKING_CHECKS=1) as well as the assertion-free one. Before, the forced-mechanism kernel aborted with SIGABRT under assertions and exited 1 with the diagnostic without them. After, both configurations exit 1 and emit the diagnostic, with no abort in either. lit under assertions goes from 1659 passed with 3 failing to 1660 passed with 2 failing; sync_mechanism_field.pto now passes and the remaining two failures are the separate coverage-digest problem. The assertion-free build is unchanged at 1662 passed with none failing. ctest is 50 of 50 in both configurations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d a version The anchor signature was llvm::hash_value(signatureText). That value is written to a file by one ptoas process and compared by another, and llvm::hash_value is not reproducible across processes: get_execution_seed() returns the address of install_fatal_error_handler when LLVM_ENABLE_ABI_BREAKING_CHECKS is set, so address-space randomisation gives every run a different seed. Under an assertion build, where that macro follows assertions on, identical source therefore compared unequal and the G1 differential gate rejected it as an anchor-signature mismatch. Without assertions the seed is a fixed constant, which is the only reason this was never seen here. The digest is now llvm::xxh3_64bits, which takes no seed. That makes every previously written profile incomparable, so the profile carries sigver=N and a mismatch is reported as its own violation kind rather than as an anchor mismatch. The distinction is the whole point: one says regenerate a stale file, the other says the kernel changed, and conflating them sends a reader after the wrong thing. The version is checked BEFORE the signature so it wins even when the stored signature is also wrong. A profile from an older build has no sigver field, parses it as 0, and is refused. The field is appended to the end of the line, like the carried-edge fields, so existing substring pins still match. Verified in both configurations, and the digests now agree between them: per function, identical across two processes of the same build and identical between the assertion and assertion-free builds. The new test pins that an old-format profile whose signature is ALSO wrong is refused for its version and never reported as an anchor mismatch, and that a profile this build wrote round-trips clean. lit is 1663 passed with none failing and ctest is 50 of 50, in the assertion build and the assertion-free one alike. Under the assertion build the sync-filtered suite the review cited goes from 54 passed with 3 failing to 58 of 58. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eported Five conditions were computed and then consumed inside `if (debugEnabled)`, so on the default path a violation produced a warning string in a report nobody reads and a successful compile. `emit=FAILED` was a dead variable there. The production buffer-ID pass treats the same conditions as terminal. This makes the unified path agree with it. Now fatal: buffer-id demand still exceeding the pool after id reuse, invalid token nesting, buffer-id emission failure, a routed hazard whose event ops were not suppressed, and a routed hazard that got no token bracket. Each is a hang rather than a slow kernel: an out-of-pool physical id aliases another buffer's token, and a nesting violation is a get_buf on an id already held awaiting a release issued later in program order. NESTING IS NOW VALIDATED AFTER THE MERGE, WHICH IS THE POINT OF THIS CHANGE. `validateNoSamePhysicalIdNesting` ran before `mergeGetRls`, and the merge is a mutation of exactly what it validates: cancelling an rls/get pair on one (logicId, pipe) turns two point-holds of a physical id into a single hold spanning both sites. So the validator inspected a structure that was never emitted, and before the merge every hold was a point hold, making a nesting violation structurally unrepresentable -- the check could only ever catch a missing physical id. It reads `op2BufSync_` by reference, so after the merge it sees what codegen sees. This is the order BufidSyncPass has always used. The capacity re-check exists because `reuseIds` is not guaranteed to converge: it leaves its loop with the condition still true when no signature group holds two logic ids to merge. Nothing downstream catches the result -- `BufidSyncCodegen` casts the physical id to uint32_t with no range test, and the only buf-id range check lives in the IRSyncRecord overload of `checkDeviceIdLegality`, which this pass does not call. The unbracketed test sits ABOVE the emptiness guard on purpose. If bracketing finds no site for any routed cluster the map is empty and the block is skipped, while routing has already suppressed those hazards' event ops -- the total-failure form of what the test counts. The leaked and unbracketed counters are keyed on the routed set rather than on mechanism alone, so the test-only force-mechanism flag, which stamps BUFID without routing, cannot trip a failure that blames routing. PROVEN A NO-OP ON EVERYTHING THAT EXISTS TODAY. Emitted IR, exit code and sync telemetry were captured for 815 kernel-arch combinations under both an assertion build and an assertion-free one, before and after: zero differences in all six comparisons. Every newly fatal condition is already absent across the corpus, so this changes no current kernel's outcome and only removes the silence around a future one. lit is 1663 passed with none failing and ctest is 50 of 50, in both configurations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
routeBuffers counted hazards spanning both a routed and an unrouted buffer and the caller ignored the count, then stamped BUFID on every hazard touching any routed cluster via any_of. That suppresses the event ops of the unrouted half without giving it a token, so the ordering is carried by neither mechanism. The count was printed only under the debug flag, beside a "!! SPLIT-MECHANISM" marker nothing acted on. The plan is now refused whole, before the first hazard mutation. RouteToBufid is the first thing that touches model.hazards, and the check sits above it, so a rejected plan leaves the model untouched rather than half-converted. Closing routing over each buffer-hazard component would instead let the plan proceed with a smaller routed set. That is a better answer and a separate change; refusing is what makes the invariant hold now. NO INPUT EXERCISES THIS, AND THAT APPEARS TO BE STRUCTURAL. Three constructions were attempted. Subviews do produce a hazard naming several clusters -- the only such hazard anywhere in the tree is in subview_tile_native_preserve_stride, with clusters=[0,1,2,3] -- but the parent tile belongs to every one of those cliques, so every hazard on any child also names the parent's other cliques. The clusters therefore share their whole hazard set, which is the sole input to the routing decision, so they cannot disagree. The mechanism that could break that tie is partial physical overlap between distinct alloc_tile roots, which yields non-transitive aliasing and genuinely independent cliques. It is invisible here: the only comparison of physical ranges across distinct roots sits behind BaseMemInfo::hasKnownPhysicalAddresses, and nothing in the tree ever sets that flag, so three tiles at deliberately overlapping level3 addresses were reported as three separate non-aliasing clusters. So this ships as unexercised defensive code, stated plainly rather than covered by a fixture that would have to fake the condition. It stops being unexercised the moment hasKnownPhysicalAddresses is wired up, which is an argument for landing the refusal first. Emitted IR, exit code and sync telemetry are unchanged for all 815 kernel-arch combinations under both an assertion build and an assertion-free one, measured against the same baseline used for the terminal-failure change. lit is 1663 passed with none failing and ctest is 50 of 50 in both configurations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lAddresses Two earlier commit messages state that nothing in the tree ever sets this flag true and that its only reader is therefore dead on every path. That is wrong, and the error is recorded here rather than rewritten out of those commits. d33e584 gives it as the reason the address-sharing model reads the pto.alloc_tile operand instead of the flag, calling the flag "dead on every path" and keying on it "silently vacuous". 150ac57 goes further and builds its central claim on it: that a split hazard is structurally unreachable because partial physical overlap between distinct alloc_tile roots cannot be seen. WHAT IS ACTUALLY TRUE. Two sites set the flag, PTOIRTranslator.cpp:451 for alloc_tile and :496 for alloc_multi_tile, in both cases as `knownPhysicalAddress.has_value() && isLocalAddressSpace(space)`. A local tile whose addr is a non-negative constant qualifies, which is every level3 tile. The flag's own intent covers this: the multi-tile site's diagnostic already reads "requires planner-assigned slot addresses or a constant level3 base", so "known" has always meant an address the analyzer can trust, not one a particular pass produced. The consequence matters. Partial physical overlap between distinct level3 roots IS detected, and it does produce non-transitive aliasing: three tiles at 0, 512 and 1024 with a 1024-byte footprint yield TWO cliques of two tiles each, with hazards naming both, while the same tiles at 0, 1024 and 2048 yield three cliques of one. So 150ac57's stated reason for the split hazard being unreachable does not hold. Whether the conclusion survives for a different reason is a separate question, tracked separately. HOW THE ERROR WAS MADE, since it is the reusable part. The audit grepped for the identifier and both setters pass the argument positionally and unnamed, so neither appeared. The follow-up experiment then read aliasing_tiles=1 off the cluster report and concluded overlap was not detected -- but that report stops after six clusters, and the six shown were unrelated tiles. The overlapping ones were never printed. No behaviour changes here. The address-sharing model still reads the alloc_tile operand, which agrees with the flag on level3; the comment now says so instead of claiming the flag does not work. The flag's reader is documented with both of its sources and with the non-transitive aliasing that only it can see, which is where a reader would have to look to avoid repeating this. lit is 1663 passed with none failing in the assertion and assertion-free builds alike. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e token id A buffer-id token orders a producer before a consumer only if both ends acquire the same id. The overflow fixtures pin the emission counts -- routed_buffers, get, rls, bracketed_hazards, leaked_event_ops -- but nothing pinned the ids themselves, so a change that preserved every count while permuting ids would have passed the entire suite while ordering nothing. This pins the correspondence directly: the id on the producing bracket is captured and required to come back on the consuming bracket further down the function, and the routed direction is asserted to retain no event ops. The kernel keeps MTE3<->V pairs for the accumulator, which does not route, so the absence check is specific to the routed direction rather than to events in general. The routing kernel is lifted into its own input file so those absence checks cover the whole output instead of colliding with the unrouted kernels that share sync_forced_overflow. WHAT THIS DOES NOT PROVE, stated in the test as well. The reviewed defect was a routed-ID set captured before optimizeSamePipeMerge rewrote logical ids. The unified pass no longer calls that merge, so the defect is gone at this commit. But adding the call back was measured on this kernel and changed nothing -- get, rls and physical_ids all identical -- so this test is not a demonstrated guard against reintroducing it. Guarding that specifically would need an input the merge actually rewrites, and none is known. The invariant it does pin is worth having on its own. lit is 1664 passed with none failing and ctest is 50 of 50, in the assertion and assertion-free builds alike. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The ignore list named build/, build_plain/ and build_plan/ individually. A second configuration was added for this work as build-assert/, which matched none of them, so a `git add -A` swept 2381 generated files into a commit -- tablegen .inc output, dependency files, DartConfiguration.tcl. They have been removed from history rather than deleted in a follow-up, so the branch never carried them. build*/ covers any similarly named directory from now on, and build-assert/ is listed explicitly so the reason is findable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds enable-unified-sync, an allocator that treats event ids, buffer
ids and barriers as three resource classes over a single interval-coloring
problem, instead of three separate mechanisms decided independently. It
replaces the mechanism/id-assignment stage of the existing sync pipeline
while reusing the same front-end hazard analysis, so a kernel's sync
decisions come from one coherent model rather than a chain of local choices.
forcing the buffer-id path produces fully correct,
self-consistent emission -- hazards routed, ids assigned, get_buf/rls_buf
paired correctly, zero leaked ops -- while the same run simultaneously
assigns event ids through the ordinary path. Multiple synchronization
mechanisms coexist correctly within a single kernel, which is the core
capability this allocator is designed to unlock.
On the current sample corpus, coverage and op-count parity with the
existing sync path hold exactly, and the absolute coverage gate reports
identical results under both paths -- this PR changes how synchronization
is decided, not what gets emitted for kernels the existing corpus already
represents.
Where this goes next: as kernels that actually pressure the id pools enter
the corpus (multi-buffered or high-occupancy workloads), the buffer-id
routing path built here is what starts engaging automatically -- no
allocator changes needed, just corpus growth. Follow-up work will extend
the reachability analysis this allocator shares with the sync gates to
reason about loop trip counts, and settle an open question about same-pipe
ordering guarantees on A3 that affects how conservative the gates need to
be.
Note: next focus is on the synchronization points themselves -- converting
event-id-based sync to buffer-id sync specifically across if/else scope
boundaries, where a hazard's set and wait can sit on opposite branches.
Update: fixes all six correctness issues raised in review, plus three earlier commits kept in and named below.
Base: f8515c3. 11 commits, 20 files, +873/-47.
What changed:
150ac57 - a dependency that spanned both a converted and unconverted buffer was being converted anyway, silently dropping the ordering on the unconverted side. That case is now rejected outright instead of being partially applied.
de48e6d - this one turned out to already be fixed by earlier work on the branch. Added a regression test so it stays fixed.
9e06a9d - several failure conditions (running out of id capacity, invalid nesting, generation failure) were being computed but only printed in debug mode, so a real failure could pass silently in normal builds. These now stop the build with a real error.
2c12592 - one synchronization mode was missing a check the others already had, so it accepted an operation it shouldn't have. Now rejected consistently across all modes, with a regression test.
f4ce8ff - the mechanism comparing two compiler runs for consistency used a hash value that isn't guaranteed to be the same between runs, so identical output could be flagged as different. Replaced with a stable hash, and added a version tag so an old saved result is recognized as outdated rather than compared incorrectly.
a41229e - a debug check was firing and crashing the program before the intended error message could be printed. Removed the check; the real error now prints as intended.
Also included, from before this review started:
Verification, on two build configurations (normal, and with debug checks enabled):
tests 1664 passed, 0 failed 1664 passed, 0 failed
ctest 50/50 50/50
filtered suite 60 passed, 0 failed 60 passed, 0 failed
Worth knowing:
Python wheel packaging for ptoas, in manylinux docker image #1's fix has no current input that actually triggers it - checked carefully, this is because the situation it guards against can't currently occur, not because it wasn't tested properly.
The new test for (docker) fix platform on manylinux wheel #2 pins real behavior but isn't proven to catch the exact regression it's meant to guard against.