[Tools] Add architecture-general ISA resource diff tool

[Phil-amd]

Compare per-kernel register, spill, scratch, and LDS usage between two
FLYDSL_DUMP_IR dump directories or JSON snapshots, to expose resource
changes that functional tests do not surface.

Read register counts from the per-kernel .set <kernel>.num_vgpr and
.num_agpr symbols, which LLVM emits on every AMDGPU target, rather
than the CDNA-only .agpr_count metadata field, and count LDS traffic
under either spelling, since gfx11 renamed ds_read to ds_load. Both are
required for the tool to work outside CDNA.

Report each metric as a value, as not applicable, or as unparsed, and
block a comparison only on the last. Make vgpr the single VGPR-family
trigger, since the metadata total already includes the accumulators on
gfx90a and later. Exit 0, 1, or 2 for no regression, a regression, or an
untrustworthy result; a crash never reports as 1.

Covered by a backend-agnostic test that generates a CDNA and an RDNA dump
shape, and exposed to agents as the isa-resource-diff skill.

[jli-melchior]

gfx1250 is based on cdna5 arch.

[Phil-amd]

Done
You're right, and thanks — the label was wrong. Fixed in 8ef12c8.
Rather than relabel it cdna5, I dropped the microarchitecture names from this code entirely, because the parser never branches on the product family. It branches on two ISA properties.

[jhinpan]

Requesting changes because the new fail-closed exit-code contract has two paths that still return RESULT: OK for inputs the tool cannot safely compare. I reproduced both on head b0f3f3dd: LLVM's normal *-unknown-gfx* target spelling bypasses architecture comparison, and an unparseable *final_isa.s is omitted from a multi-kernel tree as only a warning. The focused unit test, repository checks, Python style check, and legacy-spelling scan otherwise pass.

[jhinpan]

[blocking] This split only recognizes the abbreviated amdgcn-amd-amdhsa--gfx950 form used by the synthetic test. LLVM also emits target IDs such as amdgcn-amd-amdhsa-unknown-gfx950; for those, parse_target_id() returns processor=None. Because compare_snapshots() rejects an architecture mismatch only when both processors are known, an unknown-gfx942 snapshot compared with unknown-gfx950 currently returns exit 0 / RESULT: OK. Please extract the trailing concrete gfx... processor from both LLVM spellings (and fail closed when it is genuinely unavailable), with a regression test proving a 942/950 mismatch exits 2.

[Phil-amd]

Done in b491815e. Processor now comes from the last - separated field, so both spellings parse.

The guard needed fixing too, otherwise the parse fix alone would not have closed it: with both processors None, processor != processor is false, so it never reached the known check. It now blocks whenever a processor is unidentified and the target IDs differ.

Tests: both spellings, 942 vs 950 → exit 2, gfx11-generic vs gfx12-generic → exit 2.

[jhinpan]

[blocking] A discovered *final_isa.s that yields no records is omitted as a warning, so the comparison can become silently partial. I reproduced this with one valid dump plus one empty/truncated final_isa.s on each side: both bad files are skipped and diff returns exit 0 / RESULT: OK for the remaining kernel. That contradicts the stated fail-closed contract. Please record this as a problem (or retain a blocked sentinel entry) so any final-ISA file the tool cannot parse forces exit 2, and cover the mixed valid/invalid tree case.

[Phil-amd]

Done in b491815e. Recorded as a problem instead of a warning, so it reaches exit 2 — and summarize/snapshot fail closed on it too, via Snapshot.trustworthy.

Took the problem over a sentinel entry: a sentinel on both sides yields one blocked record per metric, and on one side it degrades into ONLY IN AFTER.

Test covers the mixed valid/invalid tree, with the healthy half asserted to exit 0 on its own first.

[jhinpan]

This points readers to a Compare per-kernel ISA resources section that is not present in docs/testing_benchmarking_guide.md; this PR only adds the script to that guide's source-file table. Please either add the promised column reference or link to the documentation that actually defines these columns.

[Phil-amd]

Done in b491815e. Dropped the dangling reference and put the column table in the skill itself.

docs/kernel_tuning_guide.md:462 already pointed here for the full column reference, so the two files pointed at each other while it existed in neither. Defining it here makes that pointer true, rather than adding an ISA column reference to a testing guide.

[Phil-amd]

@jhinpan @jli-melchior
No further comments, please proceed for approval

[jhinpan]

The three findings from b0f3f3d are fixed, but the fail-closed contract still has five independently reproduced paths that report trustworthy data or RESULT: OK after losing information. The focused unit tests and repository checks pass on exact head b491815; the inline findings are additional trustworthiness gaps in the new tool.

[jhinpan]

[blocking] Processor equality does not prove the target modes are comparable. I changed only the suffixes to gfx942:xnack+ and gfx942:xnack-; this branch returned exit 0 / RESULT: OK. Removing both target declarations also returns exit 0. These modes can change code generation and resource use, so please normalize the empty-vs-unknown environment spelling but require a nonempty processor and identical feature sets, otherwise exit 2. Add regressions for differing features and two missing targets.

[Phil-amd]

Done in aa9b7307. Comparability is now decided on the parsed processor and feature set instead of the raw target ID string, so the environment field normalizes away and xnack/sramecc no longer slip through. Features are sorted on parse, since LLVM's ordering is not a contract.

Both halves confirmed: gfx942:xnack+ vs gfx942:xnack- → exit 2, xnack+ vs sramecc+:xnack+ → exit 2, no target directive on either side → exit 2, and amdgcn-amd-amdhsa--gfx942 vs amdgcn-amd-amdhsa-unknown-gfx942 stays exit 0.

One consequence worth naming: requiring a nonempty processor also refuses gfx11-generic against itself, since RE_PROC does not recognize it. That is the right default — without a processor the scratch/spill applicability is undecidable — and if generic targets ever matter here the fix is to teach RE_PROC about them, not to loosen the verdict. Pinned in the tests either way.

[jhinpan]

[blocking] This silently drops a metadata entry with neither .name nor .symbol; the resulting snapshot still has trustworthy=True. records[name] = ... also lets a later duplicate name overwrite an earlier record, so a stale duplicate can hide a changed counter. Treat missing and duplicate kernel identities as snapshot problems that force exit 2, and cover both mutations.

[Phil-amd]

Done in aa9b7307. Both are now snapshot problems.

The identity-less case was a plain bug: _kernel_name() already returned "metadata entry has neither .name nor .symbol" and the caller discarded it before continue. parse_isa() now returns (records, file problems) so there is a place for a fault that belongs to the file rather than to a record; collect() folds them into Snapshot.problems.

Duplicates keep the first entry rather than the last and record the collision, so a stale duplicate can no longer overwrite a changed counter silently.

Both regressions splice a second metadata entry into an otherwise healthy file — a single-entry file already raises on losing its only kernel, which hides the case you reported.

[jhinpan]

[blocking] Resource counters are non-negative, but int(raw) accepts impossible values. Changing .vgpr_count from 285 to -1 was classified as an improvement and returned exit 0. Validate the metric domain and make any negative resource count untrustworthy instead of comparable.

[Phil-amd]

Done in aa9b7307. Negative values are rejected in both _metadata_cell() and _symbol_cell(), which makes the metric unparsed and forces exit 2.

Agreed this was the worst of the five: every metric here is a count or a byte size, so a negative one means the dump is malformed, and rendering the drop toward it as an improvement is the one verdict the tool must never invent. Regression covers .vgpr_count: -1 and a negative .set symbol.

[jhinpan]

[blocking] Replacement decoding can silently erase an instruction mnemonic. Corrupting one byte in one ds_read changed the reported count from 3 to 2 while the snapshot remained trustworthy with no problems. Decode strictly, or detect replacement characters and force exit 2, rather than publishing partial instruction counts.

[Phil-amd]

Done in aa9b7307. Kept the lenient decode — a traceback reports worse than exit 2 does — but the file is now decoded strictly first, and a UnicodeDecodeError is recorded as a file problem before falling back to errors="replace". So the replacement is admitted rather than counted.

Went with strict-then-fall-back over scanning for U+FFFD, which would also fire on a file that legitimately contains that character. Regression corrupts one byte inside a ds_read mnemonic and asserts both halves: the count really does drop to 2, and the snapshot is no longer trustworthy.

[jhinpan]

[blocking] Schema v2 accepts n/a for every metric. Setting mandatory vgpr to n/a on both JSON inputs bypasses comparison and returns exit 0. Validate state applicability by metric/architecture: mandatory register, spill, scratch-byte, and static-LDS counters cannot be n/a; reserve it only for quantities that genuinely do not exist on that target.

[Phil-amd]

Declining this one, unlike the other four.

n/a is only ever produced by _instruction_cells(), for scratch_store/scratch_load on a target that spills through buffer_*. No path in this tool — or in LLVM — emits n/a for vgpr, and Cell.from_json() already validates the state enum, the value type and the reason type. Reaching the case you describe means hand-editing the tool's own serialized output, which is not a trustworthiness gap in the data path the way the other four are: those all start from a file LLVM or the filesystem produced.

The prescribed fix also has a cost I would rather not pay. "Validate state applicability by metric/architecture" puts a second copy of the applicability rules in the loader, so spills_via_scratch() and the JSON validator would have to be kept in agreement forever, and the first time they drift the tool refuses a snapshot it wrote itself.

Happy to reconsider if you can show a snapshot the tool actually produces that carries n/a on a mandatory metric, or a second producer of this schema — either would move it into the same class as the rest.

[Phil-amd]

@jhinpan
Continue review when you have time.

[jhinpan]

Reviewed current head a3e74b4 in three passes (parser/data integrity; CLI/schema/adversarial inputs; tests/docs/CI). The focused unit suite passes 9/9, and check_repo plus Python style both pass. The Navi CI failure is an unrelated runner OOM (only 122 MiB free). However, the reusable JSON path still violates the fail-closed contract: the previously reported case where both snapshots mark a mandatory metric as n/a still returns RESULT: OK, and Cell.from_json() also accepts negative resource values, so a 10 -> -1 VGPR change is reported as an improvement with exit 0. Please make snapshot loading validate metric state/domain and add CLI-level JSON regressions for both cases. No GPU run was needed because this PR is compile-only tooling and both failures reproduce deterministically through its CLI.

[jhinpan]

[blocking] The assembly parser now rejects negative counters, but the reusable JSON decoder still accepts them. I reproduced a schema-v2 snapshot with vgpr=-1: load_snapshot() marked it trustworthy and diffing 10 -> -1 returned exit 0 / RESULT: OK. Please reject negative VALUE cells during snapshot loading (and add a CLI-level JSON regression). This is the same fail-closed boundary as the still-unresolved mandatory-n/a case, where two snapshots both declaring vgpr as n/a also return OK.

[Phil-amd]

Split verdict on the two halves.

Negative VALUE cells on load — accepted, fixed in 7eff378a. You are right that this was an asymmetry I introduced: I put the domain check in _metadata_cell()/_symbol_cell() and left Cell.from_json() without it, for the same Cell type and the same invariant. Non-negativity belongs to the metric, not to the parser that happened to read it. Cell.from_json() now raises SnapshotError on a negative value, consistent with how it already rejects a wrong type there.

CLI-level regression added as asked: diff ten.json negative.json → exit 2, with a healthy JSON pair asserted at exit 0 first so the 2 is attributable. Mutation-checked — disabling the new branch fails exactly that test.

Mandatory n/a — still declining, for the reason given last round, which this review does not address. The distinction is domain versus applicability. Non-negativity is intrinsic to the value and is one comparison in the place the value is constructed, which is why I took it. Which metrics may be n/a is a property of the target, derived by _instruction_cells() from spills_via_scratch(); asserting it in the loader means a second copy of that rule, and the first time the two drift the tool refuses a snapshot it wrote itself.

I asked for one of two things to move it into the same class as the rest: a snapshot the tool actually produces that carries n/a on a mandatory metric, or a second producer of this schema. Re-running the same hand-edited input is not either of those — Cell.na() is called in exactly one place, for scratch_store/scratch_load, and no path in this tool or in LLVM writes n/a for vgpr. If you have a concrete producer in mind I will take it.

Unrelated but worth flagging: this review was against a3e74b4, which was a force-push of the same commit onto a newer main from another machine. I rebased onto it rather than over it, so nothing from that push was dropped — the only delta from a3e74b4 is the fix above.